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

Fix lambda cross-thread dynamics (take 2) #85248

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

RandomShaper
Copy link
Member

This PR has two commits:

  1. Reverts recent attempts to fix a chain of issues, which was making the whole thing extremely complex.
  2. Implements an alternative to Fix lambda cross-thread dynamics (reverted) #84659 (the first reverted one), much simpler, now I've been lucid enough to realize how to do it properly. I'm muuuch more satisfied with this approach. (Reviewers are advised to check the diff of just this one, to be back to the starting point.)

Supersedes #85223.
Fixes #85151.

Tested the following recent related bugs are still gone after this PR (plus no address sanitizer issues detected);

@RandomShaper RandomShaper force-pushed the fix_gdlamb_doublefree_2 branch from edd80c1 to a34aa4e Compare November 23, 2023 09:25
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
@RandomShaper RandomShaper force-pushed the fix_gdlamb_doublefree_2 branch from a34aa4e to 7e99a6e Compare November 23, 2023 09:37
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I left a couple of questions, but overall it feels good and this does look simpler. I can almost understand what's going on 🙃

I tested #85151 and #85112, which I was able to reproduce before, and can confirm this PR fixes/refixes them.

@RandomShaper RandomShaper force-pushed the fix_gdlamb_doublefree_2 branch from 7e99a6e to bfe66ab Compare November 23, 2023 17:50
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested successfully on a few projects using lambdas.

@akien-mga akien-mga merged commit 070ac8d into godotengine:master Nov 23, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

GDScript engine destruction causes deadlock when destroying lambdas
5 participants