mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
527 stars 119 forks source link

fix(MockHttpSocket): exhaust .write() callbacks for mocked requests #542

Closed kettanaito closed 3 months ago

kettanaito commented 3 months ago

Todos

kettanaito commented 3 months ago

Hi, @mikicho 👋 This is the fix for the finish event not being emitted for mocked requests. Can you please give it a look and see if you spot anything off? Thanks.

mikicho commented 3 months ago

I just tested it, and it fixed some tests, YaY. However, I encountered an interesting time-based edge case that our tests caught. I'm not sure if this is the expected behavior, but somehow, Nock preserves the same event order as Node. Anyway, if you think about something quickly, it should be cool; if not, we may revisit it later.

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)

➜ nock git:(Michael/fetch-support) ✗ node a.js unmocked [ 'socket', 'prefinish', 'finish', 'response', 'close', 'abort' ] ➜ nock git:(Michael/fetch-support) ✗ node a.js mocked [ 'socket', 'prefinish', 'finish', 'response', 'close', 'abort' ]

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)

➜ nock git:(Michael/fetch-support) ✗ node a.js mocked [ 'socket', 'prefinish', 'finish', 'response', 'close', 'abort' ] ➜ nock git:(Michael/fetch-support) ✗ node a.js unmocked [ 'socket', 'prefinish', 'finish', 'abort', 'error', 'close' ] ➜ nock git:(Michael/fetch-support) ✗

For small abort timeout, we emit response event that Node.js doesn't.

kettanaito commented 3 months ago

@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.

mikicho commented 3 months ago

@kettanaito Agreed.

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.

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? https://github.com/mswjs/interceptors/pull/542#discussion_r1545794943

kettanaito commented 3 months ago

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 x-request-id string, which broke the XHR -> ClientRequest interceptors deduplication and resulted in extra request/response events on the interceptor.

mikicho commented 3 months ago

It fixed more tests! image

(P.S.: Some tests have revealed that Nock is not behaving properly, and the new interceptor has found the issue!)

kettanaito commented 3 months ago

That is the best kind of discovery! Yohoo! Should we merge this?

mikicho commented 3 months ago

image