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.

Sign up for GitHub

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

Jump to bottom

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
Contributor

@alixander alixander commented Dec 18, 2023 •
edited
Loading

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.

@alixander alixander mentioned this pull request Dec 18, 2023
Merged
@nhooyr
Copy link
Contributor

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 nhooyr mentioned this pull request Dec 19, 2023
Closed
@nhooyr
Copy link
Contributor

nhooyr commented Dec 19, 2023

Didn't get a chance, hopefully tomorrow.

@nhooyr
Copy link
Contributor

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
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
Contributor

nhooyr commented Feb 22, 2024

I know it's on my radar.

@haha454 haha454 mentioned this pull request Feb 22, 2024
Closed
nhooyr added a commit to alixander/websocket that referenced this pull request Apr 5, 2024
@nhooyr
close.go: Rewrite how the library handles closing
231f9ae 
Far simpler now. Sorry this took a while.

Closes coder#427
Closes coder#429
Closes coder#434
Closes coder#436
Closes coder#437
@nhooyr
Copy link
Contributor

nhooyr commented Apr 5, 2024

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

@nhooyr nhooyr changed the base branch from master to dev April 5, 2024 23:26
This was referenced Apr 5, 2024
Closed
Closed
alixander and others added 3 commits April 5, 2024 16:42
@alixander @nhooyr
fix closenow race
46f4112
@nhooyr
accept_test: Fix @alixander's test
0b3912f 
Not ideal but whatever, I'm going to rewrite all of this anyway.
@nhooyr
close.go: Rewrite how the library handles closing
db18a31 
Far simpler now. Sorry this took a while.

Closes coder#427
Closes coder#429
Closes coder#434
Closes coder#436
Closes coder#437
@nhooyr nhooyr added this to the v1.8.11 milestone Apr 5, 2024
@nhooyr
Copy link
Contributor

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.

@nhooyr
mask_asm: Disable for now until v1.9.0
c97fb09 
See coder#326
@nhooyr
ws_js_test: Fix
211ef4b
@nhooyr
read: Fix CloseRead to have its own done channel
250db1e 
Context can be cancelled by parent. Doesn't indicate the CloseRead goroutine
has exited.
@nhooyr nhooyr merged commit b0ec201 into coder:dev Apr 7, 2024
4 checks passed
@alixander alixander mentioned this pull request Apr 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v1.8.11
Development

Successfully merging this pull request may close these issues.
3 participants
@alixander @nhooyr @ClaytonNorthey92