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.

Sign up for GitHub

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

Jump to bottom

fix(ClientRequest): throw when writing after end for bypass responses #462

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(ClientRequest): throw when writing after end for bypass responses #462

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
103f8c3
failing test
mikicho Sep 30, 2023
8204bd9
fix tests
mikicho Oct 3, 2023
def1a6b
Merge remote-tracking branch 'upstream/main' into Michael/no-write-af…
mikicho Oct 4, 2023
896f6c2
fix
mikicho Oct 4, 2023
fe9fe5d
Merge remote-tracking branch 'upstream/main' into Michael/no-write-af…
mikicho Dec 1, 2023
ad67406
fix
mikicho Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/interceptors/ClientRequest/NodeClientRequest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,3 +315,26 @@ it('does not send request body to the original server given mocked response', as
const text = await getIncomingMessageBody(response)
expect(text).toBe('mock created!')
})

it.only('emits the ERR_STREAM_WRITE_AFTER_END error when write after end given no mocked response', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a compliance test in test/modules/http/compliance where we will also feature the real usage of http.* instead of constructing the internal class.

const emitter = new Emitter<HttpRequestEventMap>()
const request = new NodeClientRequest(
normalizeClientRequestArgs('http:', httpServer.http.url('/write')),
{
emitter,
logger,
}
)

const errorReceived = new DeferredPromise<NodeJS.ErrnoException>()
request.on('error', (error) => {
errorReceived.resolve(error)
})

request.end()
request.write('foo')

const error = await errorReceived

expect(error.code).toBe('ERR_STREAM_WRITE_AFTER_END')
})
5 changes: 5 additions & 0 deletions src/interceptors/ClientRequest/NodeClientRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ export class NodeClientRequest extends ClientRequest {
}

write(...args: ClientRequestWriteArgs): boolean {
// Fallback to native behavior to throw
if (this.destroyed || this.finished) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe neither of these properties will be set immediately after calling .end() for bypassed requests because they are set by Node in super.end() which is called asynchronously.

Given the current limitations, I think the most straightforward way to fix this is to have an internal this.requestSent boolean flag that will control whether .write() calls should be buffered anymore. We can set that flag to true immediately in .end() regardless of the request listener lookup result.

write(...) {
  if (this.requestSent) {
    throwNodeWriteAfterEndError()
  }
}

end(...) {
  // Mark the request as sent immediately so
  // the interceptor would know to stop buffering
  // ".write()" calls after this point: that's a no-op.
  this.requestSent = true
}

What do you think about this?

I dislike introducing any new state but I don't suppose we can rely on request properties since those we need are set async anyway:

  • finished is set by respondWith() in case of mock responses.
  • finished is set by Node by super.end() in case of bypassed responses.

And both happen asynchronously after the request listener promise settles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I dislike this as well.

We can alternate the finished property:

end(..) {
  this.finished = true

  until(...).then(() => {
    this.finished = false
  })
}

But I think this is more confusing and error-prone than a new state.

Copy link
Member

Choose a reason for hiding this comment

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

Alas, we can't finished means request body has been sent. In the case of original requests, calling .end() doesn't mean that the request body has been sent—it will start sending in the .end(). We do have to rely on an internal custom flag for this.

Copy link
Member

Choose a reason for hiding this comment

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

And we'd likely have to recreate the original Node.js error here because calling super.write() in this case won't produce the "can't write after end error". It will very likely produce something connection-related because the connection has failed to establish.

It shouldn't be hard replicating the Node's native error.

return super.write(args)
}

const [chunk, encoding, callback] = normalizeClientRequestWriteArgs(args)
this.logger.info('write:', { chunk, encoding, callback })
this.chunks.push({ chunk, encoding })
Expand Down