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

fix: MockHttpSocket: if passthrough: dont flush write buffer on stream read #659

Closed

Conversation

thecopy
Copy link

@thecopy thecopy commented Oct 10, 2024

resolves mswjs/msw#2309

Underlying issue
As i understand the code: if consumer attempts to start reading stream before buffer is sent, the items will be removed from the writeBuffer. If passthrough() is called after, it will miss those chunks.

This change disables calling flushWriteBuffer on stream.read if current request is a passthrough request.

@kettanaito
Copy link
Member

Hi, @thecopy. Thanks for opening this.

If we don't flush request chunks on read(), then if you try to read the request's body in your handler, it will pend forever.

Note that it's the handler that decides whether the request is mocked or passthrough, not the other way around. In other words, you cannot know if the request is passthrough until you flushed the request buffer on read so the handler can read the request body and decide on how to handle the request.

What you are describing sounds like a bug in how we are handling the write buffer. We should still flush, but if the passthrough is established, we should send those chunks via the original socket instance as well to make a proper request. Does that sound right to you? Wdyt?

@@ -63,6 +63,7 @@ export class MockHttpSocket extends MockSocket {
private responseType: 'mock' | 'bypassed' = 'bypassed'
private responseParser: HTTPParser<1>
private responseStream?: Readable
private isPassthrough: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Cannot this be represented via responseType === 'bypassed? We already have a flag to track this.

@@ -472,7 +477,9 @@ export class MockHttpSocket extends MockSocket {
// 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()
if (!this.isPassthrough) {
this.flushWriteBuffer()
Copy link
Member

Choose a reason for hiding this comment

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

As I've mentioned, this introduces an impossible state. You won't know if the request is passthrough until the handler is finished running. For the handler to finish running, it must be able to read the request's body, for which we must flush these first chunks.

@kettanaito
Copy link
Member

Released: v0.36.10 🎉

This has been released in v0.36.10!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants