- Notifications
You must be signed in to change notification settings - Fork329
fix closenow race#427
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Copy, weird. Will review tonight. Have a Christmas party now. Happy Holidays! 🥳 |
Didn't get a chance, hopefully tomorrow. |
I despise the way this code was implemented. This is one of multiple races. See also#239 But this looks like the right fix thanks@alixander I'll take a look at the test tonight. Helping cook Christmas meals at the Legion today. |
Merry Christmas man! Christmas in small towns sound 💯 . I'm sure the test isn't up to par lol. Happy to address comments but also if you wanna just push to this or redo this in another PR, I'm cool with it. |
ClaytonNorthey92 commentedFeb 21, 2024
hey@alixander and@nhooyr , is it possible to merge this fix in? I am running into this issue on one of my projects. thanks! |
I know it's on my radar. |
Will have a release out by Sunday to fix this and all the associated bugs. |
Not ideal but whatever, I'm going to rewrite all of this anyway.
Not sure why the Wasm test is failing, looking into it. Not sure if it's related to this PR or something else as it seems to fail intermittently in the daily CI. |
Context can be cancelled by parent. Doesn't indicate the CloseRead goroutinehas exited.
Uh oh!
There was an error while loading.Please reload this page.
the added test fails before this with
i originally found this in D2 when i added tests for watching and --race started failing but the code seems to be purely confined in this library.
hacked together test, so if you accept it and want me to clean it up, i can. just seeing if this is legit.