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

1.9.0 may need to be yanked #103

Closed
james7132 opened this issue Feb 22, 2024 · 12 comments
Closed

1.9.0 may need to be yanked #103

james7132 opened this issue Feb 22, 2024 · 12 comments

Comments

@james7132
Copy link
Contributor

james7132 commented Feb 22, 2024

bevyengine/bevy#12044 reports that the update to 1.9.0 is SemVer compatible with the prior 1.7.x and 1.8.x releases, and is currently causing all new fresh fetches and builds of Bevy to lock up on start. Likely due to a bug with #93. Still trying to root cause the issue.

@taiki-e
Copy link
Collaborator

taiki-e commented Feb 22, 2024

Thanks. I yanked 1.9.0 now.

@james7132
Copy link
Contributor Author

Thank you. I'll try to figure out why this was failing, and incorporate the findings and fixes into #102, as it does seem to fix one of the problems introduced.

@zeenix
Copy link
Member

zeenix commented Feb 22, 2024

FWIW, this version also breaks zbus.

@james7132
Copy link
Contributor Author

The problem seems to be coming from the use of thread-local AtomicWakers. Upon removing them, it seems to allow for the executor to properly work, albeit it's yielding to the OS for way too long.

@james7132
Copy link
Contributor Author

I've narrowed the problem down to the fact that the executor was storing wakers in two separate locations: the Sleepers and the thread-local AtomicWakers, if a thread was awoken by the thread-local wakers, the sleeping state of the corresponding ticker was not updated.

These two different storage mechanisms for wakers likely needs to be consolidated and synchronized in some form.

@james7132
Copy link
Contributor Author

@zeenix can you check if #102 at least fixes the deadlock? This took a quick glance at zbus and it isn't using Executor::run in any way, so this localizes the problem to just how ticking works.

@notgull
Copy link
Member

notgull commented Feb 23, 2024

Maybe my instinct here was correct, as we need to keep track of the sleeper state in one central location.

@james7132
Copy link
Contributor Author

I got Bevy to work without deadlocking. We were removing the waker from Sleepers but not from the LocalQueue when dropping Ticker.

@zeenix
Copy link
Member

zeenix commented Feb 23, 2024

@zeenix can you check if #102 at least fixes the deadlock?

I'll check..

This took a quick glance at zbus and it isn't using Executor::run in any way, so this localizes the problem to just how ticking works.

Actually, it is. :)

@zeenix
Copy link
Member

zeenix commented Feb 23, 2024

@zeenix can you check if #102 at least fixes the deadlock?

I'll check..

At least all the zbus tests pass on both Linux and Windows with your branch.

@taiki-e taiki-e mentioned this issue Mar 13, 2024
@notgull
Copy link
Member

notgull commented Mar 30, 2024

With release v1.9.1, we've reverted the thread-local queue changes. I intend to add these back; however we have to be very careful about corner cases that are also introduced by e.g. ticking the executor instead of running it.

@james7132
Copy link
Contributor Author

Unfortunate, but thanks for addressing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants