-
-
Notifications
You must be signed in to change notification settings - Fork 123
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(ClientRequest): destroy socket when destroying IncomingMessage #597
Conversation
kettanaito
commented
Jul 6, 2024
- Fixes Propagate socket error event on res.destroy #449
@@ -140,6 +140,11 @@ export class MockHttpSocket extends MockSocket { | |||
// Normally, we shoud listen to the "close" event but it | |||
// can be suppressed by using the "emitClose: false" option. | |||
this.responseParser.free() | |||
|
|||
if (error) { | |||
this.emit('error', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were missing this part. If the destroy was not graceful (contains error), emit the error event on the socket. This is precisely how Node.js behaves.
@@ -153,6 +158,12 @@ export class MockHttpSocket extends MockSocket { | |||
} | |||
|
|||
const socket = this.createConnection() | |||
|
|||
// If the developer destroys the socket, destroy the original connection. | |||
this.once('error', (error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this: if the socket gets destroyed by the developer, abort the pending actual socket connection.
@@ -308,6 +319,11 @@ export class MockHttpSocket extends MockSocket { | |||
serverResponse.removeHeader('connection') | |||
serverResponse.removeHeader('date') | |||
|
|||
// If the developer destroy the socket, gracefully destroy the response. | |||
this.once('error', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same for the mocked responses: if the socket gets destroyed, abort the pending server response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
request.respondWith(new Response()) | ||
}) | ||
|
||
const abortController = new AbortController() | ||
const request = http.get(requestUrl, { signal: abortController.signal }) | ||
|
||
await requestEmitted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning
Awaiting the request listener, for some reason, breaks this test. It's not crucial for the thing we are trying to check so I'm omitting it.
5fbf36d
to
0c65c9f
Compare
Released: v0.32.1 🎉This has been released in v0.32.1! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |