-
-
Notifications
You must be signed in to change notification settings - Fork 120
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(MockHttpSocket): exhaust .write() callbacks for mocked requests #542
fix(MockHttpSocket): exhaust .write() callbacks for mocked requests #542
Conversation
// It has been used by the request parser to construct | ||
// a Fetch API Request instance representing this request. | ||
for (const [_, __, writeCallback] of this.writeBuffer) { | ||
writeCallback?.() |
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 actual fix.
When ClientRequest writes the data to the socket, the .write()
method of the MockSocket just buffers the chunks in this.writeBuffer
. It never actually does any writes (we cannot do that until we know if the connection is possible) and so it never called any write callbacks (such as the onFinish
callback).
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.
Maybe we now can/should return false in the mocked socket.write
?
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.
Now I see we are calling the write callback after the response, which causes a deadlock because not all data is ready for the user:
const http = require('http')
const sinon = require('sinon')
const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const interceptor = new ClientRequestInterceptor()
interceptor.on('request', async function rootListener({ request }) {
console.log('before');
await request.arrayBuffer()
console.log('never get here');
request.respondWith(new Response('OK!'))
})
interceptor.apply()
const reqWriteCallback = sinon.spy()
const req = http.request(
{
host: 'example.com',
method: 'POST',
path: '/',
port: 80,
},
res => {
console.log(2)
res.on('end', () => {
console.log(3);
})
// Streams start in 'paused' mode and must be started.
// See https://nodejs.org/api/stream.html#stream_class_stream_readable
res.resume()
},
)
req.write('mamma mia', null, () => {
console.log(1);
reqWriteCallback()
req.end()
})
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.
Are you talking about the response event when you mention "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.
I mean we call the write
callback inside the responseWith
function. sorry for the confusion.
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.
Is that the expected order?
TL;DR: I think yes.
It's a design choice. If I understand you correctly, you would rather the whole request body to be ready for the user in the interceptor so that they can do await request.arrayBuffer
without a fear of deadlock like this.
We can also say that the body isn't guaranteed to be ready, and they user needs to take care of this scenario by itself (I think they can with request.arrayBuffer().then(..)
)
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.
Actually, not quite. The request event is emitted as soon as the request's headers are sent. So the request body may still be streaming. If the user decides to read it, whichever body reading method will return a Promise, and the entire request listener would have to wait for that promise.
I wonder if that conflicts with the write callbacks. As in, in order to read the request body, the socket has to call the callbacks of .write(), and we are calling those only as a part of respondWith()
.
That shouldn't be the case though. The "request body" the user is reading in the request listener is our internal request body buffer where we buffer the chunks pushed.
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.
That shouldn't be the case though. The "request body" the user is reading in the request listener is our internal request body buffer where we buffer the chunks pushed.
But the internal request body never ends because we never call the write callback, which ends the request body. (we never do: this.requestStream.push(null)
)
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.
Not sure we should be doing this. Need to look more into this behavior but it's the actual request that calls .end()
on the writable stream. Perhaps the issue is that we are not translating that to this.requestStream.push(null)
.
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.
I've pushed a fix in 7be0242.
Basically, if something attempts to read the request buffer (e.g. await request.text()
in the interceptor), the MockHttpSocket
will immediately start flushing all the write callbacks.
If nothing reads the buffer, the write callbacks will be flushed before the mocked response starts streaming or before the original request is made.
@mikicho, can you please take a look if this fixes your use case? I put it in the test also with that commit.
@@ -187,6 +188,7 @@ export class MockHttpSocket extends MockSocket { | |||
.on('prefinish', () => this.emit('prefinish')) | |||
.on('finish', () => this.emit('finish')) | |||
.on('close', (hadError) => this.emit('close', hadError)) | |||
.on('end', () => this.emit('end')) |
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 never forwarded the end
event of the original socket. I think this should be added.
} | ||
|
||
return true | ||
return super.push(chunk, encoding) |
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.
Call super.push()
for the .push(null)
to correctly transition the socket into the end state:
- emit
end
event - set
readable
tofalse
- set
readableEnded
totrue
@@ -22,6 +22,10 @@ export class MockSocket extends net.Socket { | |||
super() | |||
this.connecting = false | |||
this.connect() | |||
|
|||
this._final = (callback) => { | |||
callback(null) |
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.
Implement the _final
method so socket writes transition the socket to the right "final" state:
- set
writableFinished
totrue
- emit
finish
event
this._final()
is called onprefinish
.
expect(finishListener).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
it('emits "finish" for a mocked request', async () => { |
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.
@mikicho, this is the failing write finish test you sent me. It's passing now.
|
||
// Read the data to free the buffer and | ||
// make Socket emit "end". | ||
socket.read() |
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.
This was a broken test. We have to call .read()
to exhaust the readable stream so it emits the end
event.
Hi, @mikicho 👋 This is the fix for the |
I just tested it, and it fixed some tests, YaY. Scenario A - quick abort: const http = require('http')
const sinon = require('sinon')
const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const interceptor = new ClientRequestInterceptor()
interceptor.on('request', async function rootListener({ request }) {
request.respondWith(new Response('OK!'))
})
// interceptor.apply()
const req = http.request('http://example.com')
const emitSpy = sinon.spy(req, 'emit')
req.on('finish', () => {
setTimeout(() => {
req.abort()
}, 10)
})
req.end()
setTimeout(() => {
console.log('unmocked');
console.log(emitSpy.args.map(i => i[0]));
}, 500)
Scenario B - abort after long time: const http = require('http')
const sinon = require('sinon')
const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const interceptor = new ClientRequestInterceptor()
interceptor.on('request', async function rootListener({ request }) {
request.respondWith(new Response('OK!'))
})
// interceptor.apply()
const req = http.request('http://example.com')
const emitSpy = sinon.spy(req, 'emit')
req.on('finish', () => {
setTimeout(() => {
req.abort()
}, 1000)
})
req.on('error', err => {
// console.log(err);
})
req.end()
setTimeout(() => {
console.log('unmocked');
console.log(emitSpy.args.map(i => i[0]));
}, 5000)
For small abort timeout, we emit |
@mikicho, we should improve event compatibility but it's out of scope of this change. The order of abort/error/close events is also off compared to Node.js. Let's tackle those separately. For now, some XHR tests are failing and I need to look at why. Fail in the base feature branch also, so unlikely to be relevant to these changes. |
@kettanaito Agreed.
I saw this failing test and expected them to reflect in one of Nock's, but it didn't (although I skipped on around ~40 for now). It surprised me a little. Can you please take a look at this before merging this PR? #542 (comment) |
The failing test has been resolved in the base feature branch. I forgot to migrate the new interceptor to use a fixed request ID header vs the hard-coded |
// flush the write buffer to trigger the callbacks. | ||
// This way, if the request stream ends in the write callback, | ||
// it will indeed end correctly. | ||
this.flushWriteBuffer() |
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.
If someone starts reading the request stream, flush all the write calls immediately. This way, if the stream ends in the write callback, it will end correctly:
interceptor.on('request', async ({ request }) => {
// Start reading the request stream before
// deciding whether to mock it or not.
// This triggers "read()" on the request's ReadableStream.
await request.arrayBuffer()
})
req.write('foo', () => req.end()
That is the best kind of discovery! Yohoo! Should we merge this? |
824998b
into
feat/yet-another-socket-interceptor
finish
event for mocked responses..write()
callbacks for mocked responses.end
event when the readable ends.this.requestStream
ReadableStream. We still flush any present write calls in.respondWith()
(nothing could've read the request stream up until that point).Todos