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

[mikicho]

I added a failing test.

@kettanaito The current implementation of the end function is async (unlike in Node), which forces us to set this.finish = true in the "sync" part of it (before the return this at the end of the function) to block further writes and re-set it to false before the passthrough function if no mock is provided.
Maybe related to #346

WDYT?

[kettanaito]

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.

[kettanaito]

So, do I understand the root cause of this issue correctly:

1. Since NodeClientRequest.end returns immediately and then delegates the mock/bypass logic to an async until(), we also replay the body writes in case of bypass response in that async closure.
2. Because of that, .end() doesn't immediately mark the request as finished (request body sent), and this allows for scenarios like this without errors:

// Although request was ended, the "until()" closure may
// still be running.
req.end()
// Which allows this to be called without Node throwing an error
// because "end" hasn't really finished yet (it's async). 
req.write('should throw')

Is that about right, @mikicho?

I wonder if we can solve this by marking the request as sent immediately in .end() instead of delegating that to the until() closure. We can in mocked responses but that won't work in bypass responses because we need to replay the request body writes.

A bit of history: we buffer the .write() calls into internal chunks because writing request goes to the socket, and if the connection failed (which in case of mocked responses is expected and okay), write will throw. This way the request body buffering allows us to defer the writes until we are certain there are no mocks and the request behavior must be as it is when using ClientRequest regularly in Node.js.

With that in mind, we cannot really mark the request as finished in .end() immediately. I'm not sure if the fix you're proposing will work either since it's super.end() that marks the request as finished, and it won't be called until the until() promise fulfills (which still allows calling .write() after .end() as end is pending).

Interceptors are a bit unlike other request intercepting solutions because they don't know what requests should be mocked until requests happen (no request matching built-in). That's an intentional design choice to allow for "transparent" interception but it comes with a cost, like the async nature of the .end() method since awaiting the listeners is a potentially asynchronous actions as listeners themselves can be asynchronous (e.g. reading the request body to figure out whether the request should be mocked).

[kettanaito]

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.

[mikicho]

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.

[kettanaito]

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.

[kettanaito]

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.

[mikicho]

Is that about right

Exactly!

I'm not sure if the fix you're proposing will work either

What about manually setting the flags immediately and then unsetting them before the passthrough if the request is unmocked? This is more error-prone but should be maintained with good tests.

Interceptors are a bit unlike other request intercepting solutions because they don't know...

Yeah, I started to think about this, and then you came up with an answer! This is a real challenge indeed!
In Nock, we can know if the request is mocked synchronously.

to allow for "transparent" interception

Interesting, what do you mean by transparent interception? Without mocking (responseWith) for logging/recording/etc?

Sharing my thoughts... the current implementation answers two questions at once:

1. Whether the response is mocked? (tend to be sync)
2. What is the mocked response? (May be async)

But AFAIU it, nothing bound us to separate this question to sync and async phases. Think about something like:

interceptor.on('request', ({ request, respondWith }) => {
  respondWith(async () => {
    return await getCustomResponse(request)
  })
  console.log(request)
  return isMocked(request) // this must be sync
}) 

class NodeRequestClient {
  ...
  end() {
    const isMocked = callRequestHandler({ request, respondWith })
    if (isMocked) {
      await respondWith.resolve() // or something like that
      this.finished = true
      ... more mocking job
    } else {
      super.end()
    }
  }
}

Such implementation would significantly reduce the complexity because we won't need to buffer the writes, catch errors, and set/unset flags.

Is there a key component I'm missing? (I have a feeling I do 😅)

[kettanaito]

What about manually setting the flags immediately and then unsetting them before the passthrough if the request is unmocked?

I don't think we need to go that far. Just setting this.requestSent = true in end() will give us the right state to throw in .write() if that flag is true. We don't need to unset that flag to false ever either, regardless of what response we're about to use (we should throw after write even when using the mocked response, just like your use case reported!).

In Nock, we can know if the request is mocked synchronously.

True, this one of the differences between Nock and Interceptors. I still like the asynchronous nature of request listeners because they allow you reading the request body and basing your mocking logic around that. Reading request body is async regardless of how you represent the request, so I can only assume it's impossible to have request body-based mocks in Nock.

Interesting, what do you mean by transparent interception?

Yes! I mean that you can plug in Interceptors and they won't affect the traffic in any way by default, they will be passthrough. You can still fire side-effects, like logging/monitoring/flushing the network to a HAR, but the interception is "transparent". Perhaps not the best word but I lack the one better to describe this behavior.

nothing bound us to separate this question to sync and async phases.

We effectively introduce a duplicate state this way: having to 1) return a mocked response; 2) mark the request listener as the one returning a mocked response. I find it an inefficient API design if you have to declare your intention multiple times. With the current implementation, you have one way to state that you wish to return a mocked response: call the .respondWith() method with a Response instance. If you'd have to also manually return something from the handler in that case, it'd quickly become cumbersome and unreliable as two required things to represent the same state can get out of sync really fast.

Also consider the cases when the consumer has to perform an async action, like reading the request's body, to even know if there's a mock for the response.

Such implementation would significantly reduce the complexity because we won't need to buffer the writes, catch errors, and set/unset flags.

Complexity for us. But we have to approach it the other way around: reducing the complexity for the user. There's nothing wrong with internal complexity. In fact, most tools are a significant degree more complex and convoluted when it comes to API mocking. We are still threading on a relevantly reasonable abstraction here.

[mikicho]

so I can only assume it's impossible to have request body-based mocks in Nock.

I was wrong. Nock has body filtering.

We effectively introduce a duplicate state this way
...Complexity for us.

I agree. I'm aiming for a win-win situation

Anyway, this is out-of-scope. I'll think about it and let you know if I have a more concrete idea.
For reference, another sketch:

interceptor.on('request', async ({ request , respondWith }) => {
  console.log(await something(request)) // the hook still can be async, but we don't await for it.
  respondWith(async () => {
    return await getCustomResponse(request)
  })
}) 

class NodeRequestClient {
  ...
  end() {
    callRequestHandler({ request, respondWith })
    if(respondWith.getCalled()) {
      await respondWith.resolve() // or something like that
      this.finished = true
      ... more mocking job
    } else {
      super.end()
    }
  }
}

[mikicho]

@kettanaito Fixed. PTAL

[kettanaito]

Let's update this after #453 is merged to rely on the new this.state here:

if (this.state >= HttpRequestInternalState.Sent) {
  // Throw an error if attempting to write
  // after the request has been sent. 
}

[kettanaito]

Let me know if you need any help updating these changes. I believe they are almost done. Looking forward to having this merged!

[mikicho]

@kettanaito Sure. I'll update this tomorrow.

[mikicho]

@kettanaito fixed this reply.

It has been a while, so could you please review this PR to ensure that we haven't missed anything? Thank you!

[mikicho]

I think this also closes #460