Skip to content

Conversation

@SzabolcsGergely
Copy link
Contributor

@SzabolcsGergely SzabolcsGergely commented Jun 10, 2020

SPSC (single producer-single consumer) queue documentation about queue push function:
Requires: | only one thread is allowed to push data to the spsc_queue
We had multiple threads calling HostPipeline::onNewData once USB packet arrived, queue push wasn't protected, so the data got released in between multiple threads pushing into queue and main python thread (consumer) consuming the queue.

This issue can be reproduced easily on commit e754c2b
w/ options : ./depthai.py -s previewout left right metaout -v /dev/null
where each thread is spamming the queue -artificially- every 1 ms to reproduce the clash.
When the issue happens in the terminal it will be printed (most likely) : Invalid packet stream name!deallocated
but technically it's subject to undefined behavior due to corruption.

Protecting the queue in both consumer and producer defies the purpose of non locking queue, the purpose of this PR is not replacing the current queue implementation with a locking one, but to solve the current issue in the current context.

Copy link
Collaborator

@themarpe themarpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the second push isn't protected. Maybe rather lock the whole segment?

{
    std::unique_lock<std::mutex> guard(q_lock);
    if (!_data_queue_lf.push(host_data))
    {
        _data_queue_lf.pop();
        if (!_data_queue_lf.push(host_data))
        {
            std::cerr << "Data queue is full " << info.name << ":\n";
        }
    }
}

@SzabolcsGergely
Copy link
Contributor Author

Thanks, I was sleepy, forget to remove the unlock for extra safety.

@Luxonis-Brandon
Copy link
Contributor

Thanks, both!

Copy link
Contributor

@Luxonis-Brandon Luxonis-Brandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and works for me.

@SzabolcsGergely SzabolcsGergely merged commit 17d0aaf into master Jun 11, 2020
@SzabolcsGergely SzabolcsGergely deleted the data-packet-corruption-fix branch October 6, 2020 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants