Skip to content

[contrib/pzstd] Prevent hangs when there are errors#4080

Merged
terrelln merged 1 commit into
facebook:devfrom
yotann:pzstd_deadlock
Jan 13, 2025
Merged

[contrib/pzstd] Prevent hangs when there are errors#4080
terrelln merged 1 commit into
facebook:devfrom
yotann:pzstd_deadlock

Conversation

@yotann

@yotann yotann commented Jun 20, 2024

Copy link
Copy Markdown
Contributor

When two threads are using a WorkQueue and the reader thread exits due to an error, it must call WorkQueue::finish() to wake up the writer thread. Otherwise, if the queue is full and the writer thread is waiting for a free slot, it could hang forever.

This can happen in pratice when decompressing a large, corrupted file that does not contain pzstd skippable frames.

@yotann
[contrib/pzstd] Prevent hangs when there are errors
5dc9c3c 
When two threads are using a WorkQueue and the reader thread exits due
to an error, it must call WorkQueue::finish() to wake up the writer
thread. Otherwise, if the queue is full and the writer thread is waiting
for a free slot, it could hang forever.

This can happen in pratice when decompressing a large, corrupted file
that does not contain pzstd skippable frames.
@embg embg self-assigned this Dec 20, 2024
@embg

embg commented Jan 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Why is in->finish() here not sufficient?

zstd/contrib/pzstd/Pzstd.cpp

Line 405 in 5dc9c3c

auto inGuard = makeScopeGuard([&] { in->finish(); });

I do agree that the changes in writeFile() are necessary.

@embg

embg commented Jan 10, 2025

Copy link
Copy Markdown
Contributor

OK, I see what's going on now. In both asyncCompressChunks() and asyncDecompressFrames(), the scope guard is in the same scope as readData(). So the scope guard destructor won't run if readData() is stuck, which can cause a deadlock.

The only remaining question I have is whether it's safe to finish() a WorkQueue twice. I see you added a code comment addressing this question. Will take a look on Monday.

@embg

embg commented Jan 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

OK, I don't see any reason why we can't call finish() twice on the same WorkQueue. Locking twice is safe, setting done_ twice is safe, and notify_all() doesn't have any pre-conditions according to C++ docs.

LGTM.

@terrelln

terrelln commented Jan 13, 2025

Copy link
Copy Markdown
Contributor

Thanks for the PR @yotann & thanks for reviewing @embg!

@terrelln
terrelln merged commit 80af41e into facebook:dev Jan 13, 2025
@BrewTestBot BrewTestBot mentioned this pull request Feb 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@terrelln terrelln

@embg embg

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yotann @embg @terrelln @facebook-github-bot