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

win: fix unexpected ECONNRESET error on TCP socket #3584

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

twose
Copy link
Contributor

@twose twose commented Apr 1, 2022

Introduced by #3036.

The order of calling uv_read_stop() and uv__tcp_try_cancel_reqs() doesn't seem to matter. But calling uv_read_stop() first is more in line with the order in which the user (me) uses these APIs, and uv_read_stop() is more soft I think.

Close the tcp handle after uv_read_start() immediately without a timer can also reproduce the problem, but I still constructed a test that is closer to my example to make sure it doesn't go wrong again.

@vtjnash Thank you very much for your pointers, after spending some time reading the related source code, I also learned something, especially since I almost never read the source code under the Windows platform before.

Please correct me if I am doing something wrong :)

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Seems good to me. Surprisingly straightforward to fix.

Can you line wrap the test to 80 characters? That's the main thing to fix, and I only marked a couple lines

@twose
Copy link
Contributor Author

twose commented Apr 4, 2022

Seems good to me. Surprisingly straightforward to fix.

Also surprised...

Can you line wrap the test to 80 characters? That's the main thing to fix, and I only marked a couple lines

Fixed. It seems that the code style in the existing tests is not unified, I just found one to imitate. IMO, 80 characters seems a bit conservative nowadays. XD

@vtjnash
Copy link
Member

vtjnash commented Apr 4, 2022

What can I say haha? libuv is a conservative library.

@twose twose mentioned this pull request Apr 8, 2022
11 tasks
@vtjnash vtjnash merged commit 69ebb2d into libuv:v1.x Apr 8, 2022
@twose twose deleted the fix-unexpected-econnreset branch April 8, 2022 01:45
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants