Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vpdq] Ensure VpdqHasher joins threads on destruction #1608

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

ianwal
Copy link
Contributor

@ianwal ianwal commented Jul 27, 2024

Summary

VpdqHasher currently doesn't join its hashing threads on destruction.

This is a problem if VpdqHasher::finish() is not called and VpdqHasher is destroyed. The threads will be destroyed while not joined which causes an abort.

Also remove the template specialization code so that this file doesn't depend on FFmpeg libraries at all. These aren't necessary after #1605.

I spotted this while integrating vpdq in another project. I was using Python bindings and when an exception occurred from Python during hashing the entire program would deadlock.

Test Plan

Current regtest doesn't cover this because finish() is always called in filehasher.cpp. This would be something for a unit test.

But I tested by removing the finish() call in filehasher.cpp and ran regtest with TSan. Before there was an abort and now there isn't.

Also tested with my other project and it fixed the deadlock issue immediately.

Thanks!

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

Thanks again @ianwal for a clean writeup, including your test plans. They help a lot in making these easy to review, despite my lack of C++ knowledge!

@Dcallies Dcallies merged commit 62ada98 into facebook:main Jul 30, 2024
9 checks passed
@ianwal ianwal deleted the vpdq-fix-destructor branch July 31, 2024 01:14
@ianwal ianwal mentioned this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants