Fix lambda cross-thread dynamics (reverted) #84659
Merged
Conversation
This file contains 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
akien-mga
reviewed
Nov 9, 2023
|
Got an error while exiting the app for #84046.
Here, the loop fails because the element seems to exist in the list, even if the size of the list is 0. |
RandomShaper
force-pushed
the
fix_lambda_cross_thread
branch
from
837df82 to
eff5ee5
Compare
|
Out of draft! Given the complexity of the fix, any testing is welcome. |
RandomShaper
force-pushed
the
fix_lambda_cross_thread
branch
from
eff5ee5 to
8701499
Compare
adamscott
reviewed
Nov 9, 2023
adamscott
reviewed
Nov 9, 2023
nlupugla
reviewed
Nov 9, 2023
|
870149924abc1b36715cc90e852aa5818446a14b fixes #84046 on my machine! |
RandomShaper
force-pushed
the
fix_lambda_cross_thread
branch
2 times, most recently
from
b729ab2 to
99e8ae4
Compare
RandomShaper
force-pushed
the
fix_lambda_cross_thread
branch
from
99e8ae4 to
2715117
Compare
|
I can't say much about the implementation, but I've been testing this today with a number of projects, some relying a lot on lambdas, and so far I haven't noticed any regression. I also tested the MRP and confirmed it fixes #84046 for me. The other one seems Windows specific so I didn't test as I'm on Linux right now. |
akien-mga
approved these changes
Nov 10, 2023
|
Thanks! |
neobine
reviewed
Nov 23, 2023
akien-mga
changed the title
|
For the record, this was reverted/superseded by #85248. |
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.
While reviewing #84616 (which this one supersedes) I found the underlying issue to be more complicated.
This requires #84657 so tasks submitted to the
WorkerThreadPoolhave their thread cleanup run.Fixes #84046.
Fixes #84190.
TEMP: In draft until I can be sure both issues are really fixed.UPDATE: In a future iteration, all this thread-local bookeeping may be replaced by heap-allocated items. That would simplify this a lot, but now it's better (I think) to keep the current approach to lower the risk of regressions.