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 closenow race #427

Merged
merged 8 commits into from
Apr 7, 2024
Merged

fix closenow race #427

merged 8 commits into from
Apr 7, 2024

Conversation

alixander
Copy link
Sponsor Contributor

@alixander alixander commented Dec 18, 2023

the added test fails before this with

Screen Shot 2023-12-18 at 3 32 16 PM

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.

Screen Shot 2023-12-18 at 3 35 58 PM

hacked together test, so if you accept it and want me to clean it up, i can. just seeing if this is legit.

@nhooyr
Copy link
Owner

nhooyr commented Dec 18, 2023

Copy, weird. Will review tonight. Have a Christmas party now. Happy Holidays! 🥳

@nhooyr nhooyr added the bug label Dec 18, 2023
@nhooyr
Copy link
Owner

nhooyr commented Dec 19, 2023

Didn't get a chance, hopefully tomorrow.

@nhooyr
Copy link
Owner

nhooyr commented Dec 19, 2023

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.

@alixander
Copy link
Sponsor Contributor Author

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
Copy link

hey @alixander and @nhooyr , is it possible to merge this fix in? I am running into this issue on one of my projects.

thanks!

@nhooyr
Copy link
Owner

nhooyr commented Feb 22, 2024

I know it's on my radar.

nhooyr added a commit to alixander/websocket that referenced this pull request Apr 5, 2024
Far simpler now. Sorry this took a while.

Closes nhooyr#427
Closes nhooyr#429
Closes nhooyr#434
Closes nhooyr#436
Closes nhooyr#437
@nhooyr
Copy link
Owner

nhooyr commented Apr 5, 2024

Will have a release out by Sunday to fix this and all the associated bugs.

alixander and others added 3 commits April 5, 2024 16:42
Not ideal but whatever, I'm going to rewrite all of this anyway.
Far simpler now. Sorry this took a while.

Closes nhooyr#427
Closes nhooyr#429
Closes nhooyr#434
Closes nhooyr#436
Closes nhooyr#437
@nhooyr
Copy link
Owner

nhooyr commented Apr 6, 2024

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 goroutine
has exited.
@nhooyr nhooyr merged commit b0ec201 into nhooyr:dev Apr 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants