fix: Move logic into the thread::scope call so it doesn't hang #1040
+33
−34
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicklan
force-pushed
the
fix-thread-scope-issue-read-table-multi
branch
from
abe5d9f to
eec4c58
Compare
nicklan
commented
Jul 1, 2025
| // have handed out all copies needed, drop so record_batch_rx will exit when the last thread is | ||
| // done sending | ||
| drop(record_batch_tx); | ||
| // have handed out all copies needed, drop so record_batch_rx will exit when the last thread is |
There was a problem hiding this comment.
If you're seeing this comment as being on a change, turn on "Hide Whitespace"
nicklan
requested review from
zachschuermann,
scovich and
OussamaSaoudi
and removed request for
scovich
zachschuermann
approved these changes
Jul 1, 2025
nicklan
force-pushed
the
fix-thread-scope-issue-read-table-multi
branch
from
264f456 to
c12f44f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1040 +/- ##
=======================================
Coverage 83.74% 83.74%
=======================================
Files 92 92
Lines 21070 21070
Branches 21070 21070
=======================================
Hits 17646 17646
Misses 2509 2509
Partials 915 915 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OussamaSaoudi
approved these changes
Jul 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Previously in #957 we moved the code to use
thread::scope. This allows not having to clone an engine arc, but also means that all the threads are implicitly joined at the end of thescopecall.This meant that we waited for the threads to exit before ever sending them any work, so the version on main hangs 😱 .
Instead we need to move all the rest of the logic inside the scope call and only let the implicit joining happen at the end.
How was this change tested?
Running the program on a few tables