feat: remove socketDelay #1974

mastermatt · 2020-04-17T15:45:39Z

From a user's perspective, having both delayConnection and
socketDelay is confusing and unnecessarily.

The differences between the two are subtle, and in my experience, users
need to be fairly well versed in both the Node HTTP module and the
internals of Nock in order to accurately determine which to use.

On the surface there seems to be two use cases:

A user wants to test how their code handles a timeout event.
Whether their client is expected to abort the request, or some other
action is taken in response to the event. Either way, the user
doesn't care if wall-clock time passes, and would prefer the timeout
be simulated so their test suite can get on with it.
A user wants to force wall-clock time to pass before a response event.
This is usually to test some timeout feature that is not based on the
timeout event, or to give other code time to complete a task first.

Based on those two use cases, it seems obvious that there should be two
different delay functions, like we seem to have now. However, there are
two subtle aspects that blur the line.

When a socket emits a timeout event, which the request propagates,
the request does not end. This is true in Node and in Nock today.
Clients my choose to abort a request upon a timeout event, they may not.
In Nock today, when the socket is "applying" the artificial timeout,
to determine if it should emit a timeout, it doesn't just use the
value passed to socketDelay. It uses the sum of the values passed
to delayConnection and socketDelay. This covers the flow of a
user wanting to ensure their code sees a timeout event even if no
action is taken.

Therefore, there is no reason to have two different options for users.
The value passed to delayConnection can trigger a timeout event
right away and if the code chooses not to act on the event, then it
will wait in real time until response is triggered.

In fact, when I began working on this, I went into the
test_socket_delay.js file and replaced all socketDelay with
delayConnection. All the tests passed.

Other minor tweaks included in this change:

removeAll calls removeAllTimers
Even without the removal of socketDelay, consumers using delay
and a timeout on the socket would see the request aborted by most
clients right way, however, the ref to the timer would hang until
the callback fired.
The delay methods on the Interceptor are now just setters instead
of additive. This was undocumented, unintuitive behavior.
Fixed a bug from refactor(playback): convert all response bodies to streams #1973, where replayWithError would cause
Interceptors to be consumed twice.

BREAKING CHANGES

socketDelay has been removed. Use delayConnection instead.
delay, delayConnection, and delayBody are now setters instead of additive.
example:

nock('http://example.com')
  .get('/')
  .delay(1)
  .delay({ head: 2, body: 3 })
  .delayConnection(4)
  .delayBody(5)
  .delayBody(6)
  .reply()

Previously, the connection would have been delayed by 7 and the body delayed by 14.
Now, the connection will be delayed by 4 and the body delayed by 6.

From a user's perspective, having both `delayConnection` and `socketDelay` is confusing and unnecessarily. The differences between the two are subtle, and in my experience, users need to be fairly well versed in both the Node HTTP module and the internals of Nock in order to accurately determine which to use. On the surface there seems to be two use cases: - A user wants to test how their code handles a `timeout` event. Whether their client is expected to abort the request, or some other action is taken in response to the event. Either way, the user doesn't care if wall-clock time passes, and would prefer the timeout be simulated so their test suite can get on with it. - A user wants to force wall-clock time to pass before a `response` event. This is usually to test some timeout feature that is not based on the `timeout` event, or to give other code time to complete a task first. Based on those two use cases, it seems obvious that there should be two different delay functions, like we seem to have now. However, there are two subtle aspects that blur the line. - When a socket emits a `timeout` event, which the request propagates, the request does not end. This is true in Node and in Nock today. Clients my choose to abort a request upon a timeout event, they may not. - In Nock today, when the socket is "applying" the artificial timeout, to determine if it should emit a `timeout`, it doesn't just use the value passed to `socketDelay`. It uses the sum of the values passed to `delayConnection` and `socketDelay`. This covers the flow of a user wanting to ensure their code sees a `timeout` event even if no action is taken. Therefore, there is no reason to have two different options for users. The value passed to `delayConnection` can trigger a `timeout` event right away and if the code chooses not to act on the event, then it will wait in real time until `response` is triggered. In fact, when I began working on this, I went into the `test_socket_delay.js` file and replaced all `socketDelay` with `delayConnection`. All the tests passed. Other minor tweaks included in this change: - `removeAll` calls `removeAllTimers` Even without the removal of `socketDelay`, consumers using `delay` and a timeout on the socket would see the request aborted by most clients right way, however, the ref to the timer would hang until the callback fired. - The delay methods on the Interceptor are now just setters instead of additive. This was undocumented, unintuitive behavior. - Fixed a bug from nock#1973, where `replayWithError` would cause Interceptors to be consumed twice. BREAKING CHANGES - `socketDelay` has been removed. Use `delayConnection` instead. - `delay`, `delayConnection`, and `delayBody` are now setters instead of additive. example: ```js nock('http://example.com') .get('/') .delay(1) .delay({ head: 2, body: 3 }) .delayConnection(4) .delayBody(5) .delayBody(6) .reply() ``` Previously, the connection would have been delayed by 7 and the body delayed by 14. Now, the connection will be delayed by 4 and the body delayed by 6.

paulmelnikow · 2020-04-17T16:58:46Z

Your analysis makes sense to me. However the caveat is that I've never used this feature of Nock. It's possible we may get some comments after the fact, either from people who find the new implementation less convenient, or who have a use case which really requires a distinction.

I'm fine with making a change like this, with an openness to revisiting it later if necessary.

paulmelnikow

removeAll calls removeAllTimers
Even without the removal of socketDelay, consumers using delay
and a timeout on the socket would see the request aborted by most
clients right way, however, the ref to the timer would hang until
the callback fired.

Could you explain this more? Specifically, why is it being called from removeAll()?

paulmelnikow · 2020-04-17T16:59:50Z

README.md


 ```js
 nock('http://my.server.com')
  .get('/')
-  .socketDelay(2000) // 2 seconds
+  .delayBody(2000) // 2 seconds


I think it would be good to add a note in the readme that says .socketDelay() was removed in Nock 13.

paulmelnikow

Awesome change.

tests/test_delay.js

tests/test_nock_lifecycle.js

paulmelnikow · 2020-04-17T18:38:32Z

README.md

 ### Delay the response

+Nock can simulate response latency to allow you to test timeouts, race conditions, an other timing related scenarios.  


I'm not sure this is completely accurate, but I'd suggest something like this:

Nock can simulate the effect of response latency to allow you to test timeouts, race conditions, an other timing related scenarios. Rather than waiting for real clock time to pass it compares the requested delay to the timeout parameter passed to the overridden request methods (i.e. http.request() and http.ClientRequest#setTimeout()) and emits the timeouts almost immediately. This allows you to test timeouts without using fake timers or slowing down your tests.

The added detail here is only true when delaying the connection and a timeout is set on the request. So while I'm good with the added verbiage, I think it should go below where the details of delayConnection are covered.
I was trying to keep this opening section on delays high-level and only include the info the top 90% of users are going to want.

paulmelnikow · 2020-04-17T18:39:10Z

README.md

-```
-
-NOTE: the timeout will be fired immediately, and will not leave the simulated connection idle for the specified period of time.
+The timer starts after the [`'response'`](http://nodejs.org/api/http.html#http_event_response) event and will delay the [IncomingMessage](http://nodejs.org/api/http.html#http_http_incomingmessage) from emitting its first `'data'` or `'end'` event.


Does this one cause a real delay?

Hmm, I think it is confusing for the caller that one of these methods causes a real delay and the other one doesn't.

Maybe that was true before? Either way, since the semantics of these APIs are changing, I think it is probably a good time to look at it.

One possibility would be to change the names, e.g.:

simulateDelayedConnection(ms)

delayBody(ms)

mastermatt · 2020-04-17T18:47:46Z

removeAll calls removeAllTimers
...
Could you explain this more? Specifically, why is it being called from removeAll()?

The docs on cleanAll read as

You can cleanup all the prepared mocks (could be useful to cleanup some state after a failed test)

I've always interpreted that as this is the method to call between individual tests to ensure you don't get side effects across tests.

Currently, if a test ends while requests are mid-flight and the test suite calls cleanAll all the interceptors and Scopes are removed from the global map, but those requests are still mid-flight. And when the timers go off, they go about the request, potentially causing side-effects.

This falls into why the test "should be safe to call in the middle of a request" was added. It turns out, in our test suite, we had coverage being reported for lines that were only ever touched after tests ended.
In this case the following line only ever falls back to an empty array if removeAll had been called first.

nock/lib/intercept.js

Line 119 in c5bf54c

(allInterceptors[basePath] && allInterceptors[basePath].interceptors) || []

Tests like "socket has address() method" are completely unrelated, but because they called done while in the socket callback, removeAll would be called before the request proceeded and attempted to mark the Interceptor as consumed.

Adding removeAllTimers to cleanAll/removeAll exposed the lack of coverage while only inside running tests.
"should be safe to call in the middle of a request" was added a way to get coverage for that fallback and it tests that attempting to "remove" an interceptor after removeAll doesn't throw an error. It feels like collateral damage of only testing via the API because all I want is a test that calls removeAll then remove and show it doesn't blow up.

mastermatt · 2020-04-17T19:02:34Z

Your analysis makes sense to me. However the caveat is that I've never used this feature of Nock. It's possible we may get some comments after the fact, either from people who find the new implementation less convenient, or who have a use case which really requires a distinction.

I'm fine with making a change like this, with an openness to revisiting it later if necessary.

👍

As for "... a use case which really requires a distinction."
There is only use case that is being lost with this conversion. The ability have Nock fire a timeout event right away, but then have real-time pass before the response event.
This seems like a very rare ask from a consumer. And it would still be possible to achieve, it's just that now the consumer has to fire the timeout manually after the socket event.

It seems like a worthwhile trade off to me to have it simpler for 99% of consumers and make the rest, who by definition are already in the nitty-gritty, write 3 lines themselves.

gr2m · 2020-04-17T19:04:16Z

I trust you on this one.

If you want to try to get some feedback from users of the methods that are being removed or changed in behavior, we could release a fix version which logs a message asking for feedback, pointing to this issue.

paulmelnikow · 2020-04-17T19:12:53Z

removeAll calls removeAllTimers
...
Could you explain this more? Specifically, why is it being called from removeAll()?

The docs on cleanAll read as

You can cleanup all the prepared mocks (could be useful to cleanup some state after a failed test)

I've always interpreted that as this is the method to call between individual tests to ensure you don't get side effects across tests.

Currently, if a test ends while requests are mid-flight and the test suite calls cleanAll all the interceptors and Scopes are removed from the global map, but those requests are still mid-flight. And when the timers go off, they go about the request, potentially causing side-effects.

To me cleanAll() is not about requests in flight, but rather interceptors which haven't been matched.

What's being added feels a bit more like what .abortPendingRequests() does – a different kind of cleanup. In fact it's exactly what abortPendingRequests() is.

I think it's reasonable to have a "clean everything" method (as in the lifecycle RFC) although that seems like a separate matter.

In this case the following line only ever falls back to an empty array if removeAll had been called first.

nock/lib/intercept.js

Line 119 in c5bf54c

(allInterceptors[basePath] && allInterceptors[basePath].interceptors) || []

Tests like "socket has address() method" are completely unrelated, but because they called done while in the socket callback, removeAll would be called before the request proceeded and attempted to mark the Interceptor as consumed.

Adding removeAllTimers to cleanAll/removeAll exposed the lack of coverage while only inside running tests.
"should be safe to call in the middle of a request" was added a way to get coverage for that fallback and it tests that attempting to "remove" an interceptor after removeAll doesn't throw an error. It feels like collateral damage of only testing via the API because all I want is a test that calls removeAll then remove and show it doesn't blow up.

The rest of this makes sense.

Comment and doc updates.

mastermatt · 2020-04-19T01:45:36Z

@paulmelnikow I agree with you on the removeAllTimers topic. I've removed it from removeAll, but added a call to abortPendingRequests in the global afterEach for our test suite.
Doing so also exposed the lack of branch coverage I was seeing before.

I expanded the comment on the new test to hopefully clarify the why/what/how.

And I took another pass on the updates to the README.
I'm trying to follow the pattern of giving each method a high-level overview followed by a code example followed by in-depth details. And I beefed up the details for delayConnection.
Specifically, I tried to cover the fact that all the delay methods will cause parts of the response to be delayed in real-time. Only the timeout event is emitted almost immediately.

Also, this is the last breaking change I had lined up when I asked for a beta a few weeks ago.
My apologies on not getting through them quicker. Crazy times and all.

mastermatt · 2020-04-28T21:12:52Z

@paulmelnikow have you been able to look at this PR again?
I'm curious if the README updates make more sense now.

paulmelnikow · 2020-04-29T15:23:02Z

Yea, lemme give it another read. This has been a really busy time but I think I can read it sometime in the next two days. Thanks for the reminder!

paulmelnikow · 2020-05-02T03:18:32Z

@paulmelnikow I agree with you on the removeAllTimers topic. I've removed it from removeAll, but added a call to abortPendingRequests in the global afterEach for our test suite.
Doing so also exposed the lack of branch coverage I was seeing before.

👍

Also, this is the last breaking change I had lined up when I asked for a beta a few weeks ago.
My apologies on not getting through them quicker. Crazy times and all.

No worries! I've been feeling it too. Sorry for my delay too. My open-source time has been going to Shields these past couple weeks. We just did a big migration to Heroku. The migration was easy but I'm onboarding a new ops team too, and negotiating things with the outgoing sysadmin. As usual the people work is harder than the tech work.

paulmelnikow

One question left about the readme.

paulmelnikow · 2020-05-02T03:21:24Z

README.md

-### Delay the connection
+While all of Nock's delay functions will delay real clock time for the request/response lifetime,
+`timeout` events are emitted almost immediately by comparing the requested connection delay to the timeout parameter passed to `http.request()` or `http.ClientRequest#setTimeout()`.  
+This allows you to test timeouts without using fake timers or slowing down your tests.


These two things feel like contradictory:

"While all of Nock's delay functions will delay real clock time for the request/response lifetime"

"This allows you to test timeouts without using fake timers or slowing down your tests."

How is it that they're both true?

How is it that they're both true?

Because a timeout event does not directly affect the request or response.
Maybe "lifetime" is the wrong wording.

The timeout event is emitted "immediately", if the client chooses to not take an action (eg abort the request) the request and response will continue on as normal, after real time as passed.
Which is identical behavior to how Nock behaves now when a caller uses delayConnection and sets a timeout on the request (without socketDelay).

As for not slowing down a users tests, this assumes their code or client does take action on the timeout event and subsequently the test can finish quick.

Ahhh I see. I think putting the explanation of real clock time at the end would make it clearer. How about something like this:

Nock emits timeout events almost immediately by comparing [...] This allows you to test timeouts without using fake timers or slowing down your tests. If the client chooses to not take an action (e.g. abort the request), the request and response will continue on as normal, after real clock time has passed.

paulmelnikow · 2020-05-02T03:23:46Z

tests/setup.js

@@ -16,6 +16,7 @@ chai.use(sinonChai)

 afterEach(function () {
  nock.restore()
+  nock.abortPendingRequests()


README.md

paulmelnikow

One suggested edit. Otherwise good to go!

Co-authored-by: Paul Melnikow <github@paulmelnikow.com>

github-actions · 2020-05-02T13:58:29Z

🎉 This PR is included in version 13.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2020-06-15T16:12:41Z

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2020-06-28T16:49:57Z

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mastermatt added the breaking-change label Apr 17, 2020

mastermatt requested review from gr2m and paulmelnikow April 17, 2020 15:45

mastermatt added 2 commits April 17, 2020 09:49

fixup

f8f8d09

fixup: remember the type tests

5831908

paulmelnikow reviewed Apr 17, 2020

View reviewed changes

tests/test_delay.js Show resolved Hide resolved

tests/test_nock_lifecycle.js Show resolved Hide resolved

paulmelnikow reviewed Apr 17, 2020

View reviewed changes

Remove removeAllTimers from removeAll.

02c1181

Comment and doc updates.

This was referenced Apr 19, 2020

Weekly Digest (12 April, 2020 - 19 April, 2020) #1975

Closed

Weekly Digest (19 April, 2020 - 26 April, 2020) #1980

Closed

paulmelnikow reviewed May 2, 2020

View reviewed changes

tests/setup.js

@@ -16,6 +16,7 @@ chai.use(sinonChai)

afterEach(function () {

nock.restore()

nock.abortPendingRequests()

Copy link

Member

paulmelnikow May 2, 2020

Choose a reason for hiding this comment

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

👍

mastermatt added 2 commits May 1, 2020 22:29

Merge remote-tracking branch 'upstream/beta' into remove-socket-delay

0b2c290

fixup(readme): CR feedback.

f3d2fd3

paulmelnikow reviewed May 2, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

paulmelnikow approved these changes May 2, 2020

View reviewed changes

Update README.md

1dad3ce

Co-authored-by: Paul Melnikow <github@paulmelnikow.com>

mastermatt merged commit 6a5a3cf into nock:beta May 2, 2020

mastermatt deleted the remove-socket-delay branch May 2, 2020 13:57

github-actions bot added the released on @beta label May 2, 2020

weekly-digest bot mentioned this pull request May 3, 2020

Weekly Digest (26 April, 2020 - 3 May, 2020) #1991

Closed

github-actions bot added the released on @next label Jun 15, 2020

github-actions bot added the released label Jun 28, 2020

EmilyMaryon mentioned this pull request Aug 14, 2020

Upgrading nock from v12 to v13 causes socket close event to be called when it wasn't previously #2072

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove socketDelay #1974

feat: remove socketDelay #1974

mastermatt commented Apr 17, 2020

paulmelnikow commented Apr 17, 2020

paulmelnikow left a comment

paulmelnikow Apr 17, 2020

paulmelnikow left a comment

paulmelnikow Apr 17, 2020

mastermatt Apr 17, 2020

paulmelnikow Apr 17, 2020

mastermatt Apr 17, 2020

paulmelnikow Apr 17, 2020

mastermatt commented Apr 17, 2020

mastermatt commented Apr 17, 2020

gr2m commented Apr 17, 2020

paulmelnikow commented Apr 17, 2020

mastermatt commented Apr 19, 2020

mastermatt commented Apr 28, 2020

paulmelnikow commented Apr 29, 2020 •

edited

Loading

paulmelnikow commented May 2, 2020

paulmelnikow left a comment

paulmelnikow May 2, 2020

mastermatt May 2, 2020

paulmelnikow May 2, 2020

paulmelnikow May 2, 2020

paulmelnikow left a comment

github-actions bot commented May 2, 2020

github-actions bot commented Jun 15, 2020

github-actions bot commented Jun 28, 2020

		### Delay the response

		Nock can simulate response latency to allow you to test timeouts, race conditions, an other timing related scenarios.

feat: remove socketDelay #1974

feat: remove socketDelay #1974

Conversation

mastermatt commented Apr 17, 2020

paulmelnikow commented Apr 17, 2020

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermatt commented Apr 17, 2020

mastermatt commented Apr 17, 2020

gr2m commented Apr 17, 2020

paulmelnikow commented Apr 17, 2020

mastermatt commented Apr 19, 2020

mastermatt commented Apr 28, 2020

paulmelnikow commented Apr 29, 2020 • edited Loading

paulmelnikow commented May 2, 2020

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow left a comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2020

github-actions bot commented Jun 15, 2020

github-actions bot commented Jun 28, 2020

paulmelnikow commented Apr 29, 2020 •

edited

Loading