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

abort might not work #650

Closed
ronag opened this issue Mar 31, 2021 · 7 comments · Fixed by #689
Closed

abort might not work #650

ronag opened this issue Mar 31, 2021 · 7 comments · Fixed by #689
Assignees
Labels
bug Something isn't working
Milestone

Comments

@ronag
Copy link
Member

ronag commented Mar 31, 2021

Reviewing abort and looking at some services in production I believe aborting requests might not fully work, i.e. the socket is not killed.

@ronag ronag added the bug Something isn't working label Mar 31, 2021
@ronag ronag self-assigned this Mar 31, 2021
@ronag
Copy link
Member Author

ronag commented Mar 31, 2021

Actually this might be working as design. An aborted requests that is not in the head of the pipeline will wait for the first head to finish before killing the socket.

@mcollina
Copy link
Member

Actually this might be working as design. An aborted requests that is not in the head of the pipeline will wait for the first head to finish before killing the socket.

I agree

@ronag
Copy link
Member Author

ronag commented Apr 1, 2021

I'm still seeing leaking connections in production. Not sure if user or undici error. Trying to bypass undici Pool and Agent and just use Client with a simpler Pool/Agent logic to see if that makes any difference.

@ronag
Copy link
Member Author

ronag commented Apr 1, 2021

That made things a lot better. I suspect we have some form of edge case issue in undici. I will do an in depth review and see if I can add more tests.

@ronag ronag added this to the 4.0 milestone Apr 1, 2021
@ronag
Copy link
Member Author

ronag commented Apr 4, 2021

@mcollina @dnlup I think this is a blocker for v4 release. I haven't been able to reproduce it outside of production system.

I guess our only choice is to do in depth review and see if we can find any path that could cause this.

I'm probably most familiar with this logic and will focus on this.

@ronag
Copy link
Member Author

ronag commented Apr 6, 2021

Actually this might be working as design. An aborted requests that is not in the head of the pipeline will wait for the first head to finish before killing the socket.

I agree

There is a possibility of a deadlock here if the first request is waiting for the aborted second request (pipelining > 1). It might be safer to just destroy the entire pipeline. @mcollina wdyt?

@mcollina
Copy link
Member

mcollina commented Apr 6, 2021

There is a possibility of a deadlock here if the first request is waiting for the aborted second request (pipelining > 1). It might be safer to just destroy the entire pipeline.

Go for safety, but document this as a downside of pipelining.

ronag added a commit that referenced this issue Apr 6, 2021
ronag added a commit that referenced this issue Apr 7, 2021
ronag added a commit that referenced this issue Apr 7, 2021
@ronag ronag closed this as completed in #689 Apr 7, 2021
ronag added a commit that referenced this issue Apr 7, 2021
* fix: kill socket on abort

Fixes: #650

* fixup

* fixup

* fixup

* fixup

* fixup

* fixu

* fixuP
crysmags pushed a commit to crysmags/undici that referenced this issue Feb 27, 2024
* fix: kill socket on abort

Fixes: nodejs#650

* fixup

* fixup

* fixup

* fixup

* fixup

* fixu

* fixuP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants