-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat(ClientRequest): use net.Socket interceptor #515
Conversation
We can spy on Since the body stream is the last thing we need to call the @mikicho, what do you think about this approach? |
The N time's a charm. :)
I like the direction you are going! Would it work in the following case? const req = http.request(...)
req.write('sync') // OK
await sleep(1000)
req.write('async') // I think it will work, but I'm not sure about it. my concern is that the ClientRequest would await for the `connect` event to continue
req.end() |
It appears that the process.binding function is going to be removed. Also, a question, Would the socket-based interceptor support fetch as well? or it would still require a different interceptor? |
const self = this | ||
const originalConnect = net.Socket.prototype.connect | ||
|
||
net.Socket.prototype.connect = function (normalizedOptions) { |
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.
Should this be ...normalizedOptions
here, if it's passed on as arguments to connect?
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.
@gioragutt, I think it should be but Node.js provides here a single normalized object of options. Let's dive into the source for net.Socket.connect
to verify that's the case.
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 that's the case, how is the spread in line 44 working? JS syntax wise it wouldn't make sense otherwise
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 write about this here:
interceptors/src/interceptors/Socket/SocketInterceptor.ts
Lines 61 to 73 in a755028
/** | |
* @note In some cases, "Socket.prototype.connect" will receive already | |
* normalized arguments. The call signature of that method will differ: | |
* .connect(port, host, cb) // unnormalized | |
* .connect([options, cb, normalizedSymbol]) // normalized | |
* Check that and unwrap the arguments to have a consistent format. | |
*/ | |
const unwrappedArgs = Array.isArray(args[0]) ? args[0] : args | |
const normalizedSocketConnectArgs = net._normalizeArgs(unwrappedArgs) | |
const createConnection = () => { | |
return originalConnect.apply(this, args) | |
} |
TL;DR Node.js will normalize the args for .connect()
in some cases but not the others. That's why we get different type of args if the socket connection is called as a part of http.request()
and raw like new Socket().connect()
.
Request event duplication issueWith this approach, we have to mock the connection to the host every time in order for the This means a few different sequences of events depending on the type of the request. Request to an existing endpoint with a mocked response
Request to a non-existing endpoint with a mocked response
Request to a non-existing host without a mocked responseThis is where things get a bit tricky.
This scenario is different from the event order in the unpatched scenario. The initial In our case, the lookup/connect/ready are successful. This is problematic for a few reasons:
While no direct consumers care about the lookup/connect/ready events, the Ideas how to resolve thisOption 1: Shadow requestWe can spy on the class SocketController {
constructor(socket) {
this.socket = socket;
// Create a shadow socket.
this.shadow = new net.Socket(...socketArgs);
// Always mock the connection on the shadow.
this.mockConnect(this.shadow)
// Observe the shadow, not the original socket.
this.shadow.write = proxy
this.shadow.push = proxy
this.onShadowHasMock = (response) => {
// If the shadow socket received a mocked response
// from the "request" listener in the interceptor,
// mock the original socket connection (it doesn't matter now)
// and pipe the response to the original socket.
this.mockConnect(this.socket)
pipeResponse(response, this.socket)
}
this.onShadowHasNoMock = () => {
// If the interceptor had no mocked response for this request,
// continue with the original socket as-is.
// The original socket has been pending up to this moment.
}
}
} In order for us to still respect the original socket and not to implement a dummy Socket class to provision this "waiting" period, we can do something like an infinitely pending promise on the crucial methods of the original socket that we then resolve in case of this.socket.emit = applyProxyHere((...args) => {
waitForShadow().then(() => {
Reflect.apply(...args)
})
}) If the shadow socket has a mock, the
Problems with this solution:
|
54aa046
to
5d9a812
Compare
I tried solving the socket event order issue in f7fded8. I believe it works great, now I'm writing tests to confirm that. |
How is this propogate the
Probably not a problem, but I want to make sure I understand, in this option we will emit
It seems like |
I opened a feature request for official mocking API like undici has: nodejs/node#51979 |
It doesn't expose that data but rather hooks into those socket events to provide timings measurements. That's okay. If you take a look at the current state of the branch, it emits the events correctly for all scenarios we've talked about. There's no duplication.
This is great! I'd certainly love to hear Node's opinion on this. I would count on this landing any time soon so our work on the Socket interceptor is relevant for at least a few years.
Not anymore. |
This is impressive! how so? |
Need helpI've pushed the latest state of the changes that supports both HTTP and HTTPS requests. I've moved things around a bit, mainly made the I'm encountering errors when intercepting HTTP/HTTPS requests with a body. Error:
This indicates either that the This makes sense because right now we call I've tried things like // the interceptor
socket.once('end', () => socket.passthrough()) But they haven't fixed the problem. Isolated exampleI've put up an isolated example of this in https://github.com/kettanaito/node-tls-mock. Strangely, all tests are passing there, even for requests with body. The socket implementation seems to be the same. I advise to inspect the differences first to see if I've missed something while porting it to this branch. |
How do we need to handle What Node.js does:
Reproducible codeconst { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const http = require('http');
const interceptor = new ClientRequestInterceptor({
name: 'my-interceptor',
})
interceptor.apply();
interceptor.on('request', ({ request }) => {
request.respondWith(new Response())
});
const req = http.request({
host: 'example.com',
path: '/',
headers: { Expect: '100-continue' },
})
req.on('response', res => {
res.on('data', console.log)
res.on('end', () => { })
})
req.on('continue', () => {
console.log(1); // never get called when interceptor applied.
req.end()
}) I wonder if we should emit a |
// not connecting anymore, the response was mocked. | ||
// If the socket has been destroyed, the error was mocked. | ||
// Treat both as the result of the listener's call. | ||
if (!socket.connecting || socket.destroyed) { |
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.
So in the Socket world, the socket won't be writableEnded
until .end()
is called, which can take time (e.g. request body is streaming).
Instead, I rely on .connecting
, which we immediately set to false
now in .mockConnect()
, or the .destroyed
, which will also immediately be set to false it you mock an error.
@mikicho, we cannot cover this.
This reads to be an We should support it but we should also be careful not to prematurely support it. As in, if it shouldn't belong to the Socket level and we ignore a potential bug that prevents Node.js from calling Let's add an integration test for this first and then see! Would love to dive deeper to learn what exactly Node.js does step-by-step, and at which step it stops when using the interceptor. |
@mikicho, hi! You've mentioned there are also some remaining issues left here. Could you please post them in the comment to this pull request? I'd love to see what's left so I can tackle some of them whenever I have a moment. Thank you! |
I understand the issue. The server is the one who returns this response, and Nock mimics this behavior and emits As far as I understand,
Having said that, this looks like an esoteric edge case, so if it requires API changes, we might want to automatically |
Destroying request/response parsersWe are currently correctly destroying the parsers, specifically: The request parser is destroyed once the request emits the
The response parser is destroyed once the
|
I believe this feature is ready for merging! 🎉 The test suite passes, the compatibility looks solid. There are a few pending tasks for the Nock compatibility, which we've agreed with @mikicho to tackle after this is released. I don't wish to stall this update any longer. It's a huge change and it blocks any bugfixes with having to port them here as well. Looking forward to releasing the Socket-based interceptor sometime this week! |
Released: v0.32.0 🎉This has been released in v0.32.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Yet another attempt at the Socket-based interceptor. This time, we are tapping into the
HTTPParser
from Node.js to give us the HTTP message parsing.Roadmap
SocketController.respondWith()
. The challenge is to convert the Fetch APIResponse
to the HTTP message and push it to the socket instance. We can use https://github.com/har-tools/http-message if nothing else works.connect
andready
events are OK since those are internal and no consumers really depend on them. The main worry is the duplicate.write()
callbacks since the request body would be written twice in the case of request listener (body read 1) + a bypassed request to an existing hostname (body read 2). We can always suppress the callbacks and then replay them.Push the.mockConnect()
event until after the "request" interceptor event has been awaitedClientRequest
won't start writing the request body until it receives the"connect"
and"ready"
events from the socket.signal
Socket constructor options (I presume for aborting the connection). ConsiderAbortSignal.timeout(n)
as well. test(MockHttpSocket): add "signal" tests #559Socket.setTimeout()
that should time out the connection if it hasn't been established within the given time window. Support these for actual socket connections. test: addsetTimeout
tests #558MockHttpSocket
class).tls.connect()
, notnet.connect()
.Add compliance test for non-HTTP transfer over Socket (e.g. SMTP, see this). We should do nothing about these, it's out of scope for this feature.net.connect()
anymore. We utilizeMockSocket
to create a customhttp.Agent
and it's only applied to thehttp
module.this.requestStream
andthis.responseStream
at the right time. Consider that the consumer might've not consumed either of the streams by the time the*End()
methods onMockHttpSocket
are called (that will cause a premature close error).ClientRequest/utils
for it contains a lot of unused code now. chore: clean up ClientRequest utils #557.authorized
and.encrypyed
(see Socket: Respect "connect" and "setTimeout" behaviors #455). This is related to inheriting other properties like timeout and signal. (fix(MockHttpSocket): forward tls socket properties #556)README.md
since the interception algorithm is fundamentally different now.follow-redirect-http
test, it's pending forever (skipped in 230c238).end
event until all response event listeners are done (feat(ClientRequest): await response listeners before emitting the "end" response event #591 backport)