fix(fetch): listener leaks on Socket #32956
Conversation
Now that I think about it, there's no way to send multiple requests concurrently in HTTP/1.1 - that's only added in HTTP2. I'll revert that change. |
|
@mxschmitt could you take a look and make sure that it fixes the error in our python distro? |
This comment has been minimized.
This comment has been minimized.
| @@ -312,8 +312,11 @@ export abstract class APIRequestContext extends SdkObject { | |||
|
|
|||
| let securityDetails: har.SecurityDetails | undefined; | |||
|
|
|||
| const cleanup: (() => void)[] = []; | |||
There was a problem hiding this comment.
looks like we want eventsHelper.addEventListener instead.
There was a problem hiding this comment.
that's a nifty helper! done in a483cab
|
I wonder what does this mean for the timings? If the socket is reused, will it get zero |
This comment has been minimized.
This comment has been minimized.
Good question. I'll open a separate issue for us to investigate this.
That's what https://github.com/nodejs/node/blob/d2ad9b4fb6a6c4d54f14e19e0c74f1285ee5e402/doc/api/http.md#L60-L64 sounds like, yes. |
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"35822 passed, 619 skipped Merge workflow run. |
Closes #32951 `node:http` reuses TCP Sockets under the hood. We weren't cleaning up our listeners, leading to the `MaxListenersExceededWarning`. This PR adds cleanup logic. It also raises the warning threshhold, so that it doesn't trigger until there's 100 concurrent requests over the same socket.
Closes #32951
node:httpreuses TCP Sockets under the hood. We weren't cleaning up our listeners, leading to theMaxListenersExceededWarning.This PR adds cleanup logic. It also raises the warning threshhold, so that it doesn't trigger until there's 100 concurrent requests over the same socket.