Skip to content

Conversation

@brettporter
Copy link
Contributor

By passing in a socket timeout to the nock request, it will emit a timeout event on the request if setTimeout has been called with a lower value. It does not actually wait for the specified period of time.

I've checked that this works with the base http library's request.setTimeout and socket.setTimeout, both with a "once" function and an "on timeout" handler.

I wanted to first check if this approach makes sense, and if so I can flesh it out with tests and documentation.

This is related to #164, though for one specific case.

By passing in a socket timeout to the nock request, it will emit a timeout
event on the request if setTimeout has been called with a lower value. It
does not actually wait for the specified period of time.
@brettporter
Copy link
Contributor Author

Note: the emit error that is failing the tests is one thing that needs to be rethought - the core libraries don't actually emit an error, but they destroy the socket which will result in an error being emitted. Would appreciate your thoughts on how to handle that appropriately.

lib/socket.js Outdated
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 this is an internal function, perhaps it should be prefixed with _?

@pgte
Copy link
Member

pgte commented Mar 13, 2015

Other than this, it's looking good!
(Look out for the failing tests).

@pgte
Copy link
Member

pgte commented Mar 13, 2015

ha, I missed your comment on the failing test. Let me check this first.

@pgte
Copy link
Member

pgte commented Mar 13, 2015

Regarding the abortion, I think it shouldn't emit an error.
I think that the appropriate behaviour is similar to the way the real request.abort is implemented:

https://github.com/iojs/io.js/blob/v1.x/lib/_http_client.js#L168-L194

Aborting a request will not emit an error in node, however it will destroy
the socket being used, which then leads to an error being emitted when it is
used. Model this behaviour more accurately here.
@brettporter
Copy link
Contributor Author

Thanks for looking over it!

I think it shouldn't emit an error.
I think that the appropriate behaviour is similar to the way the real request.abort is implemented:

https://github.com/iojs/io.js/blob/v1.x/lib/_http_client.js#L168-L194

I agree. I've added some changes which instead of emitting an error on abort, it will do it if you attempt to write, end or respond to the request instead - this should match how a normal request behaves if a socket is destroyed (as is the case in the code above).

Does that make sense?

@pgte
Copy link
Member

pgte commented Mar 13, 2015

Yes it does!

@brettporter
Copy link
Contributor Author

Hi @pgte - I've added tests and documentation. Also, writing the tests made it clear that calling it socketTimeout was confusing, as you're simulating a delay / idle socket (that may or may not lead to a timeout). I've renamed it to socketDelay which is hopefully clearer and a bit closer to the other delay* methods.

Is there anything else you need?

@pgte pgte merged commit 7e6299c into nock:master Mar 16, 2015
@pgte
Copy link
Member

pgte commented Mar 16, 2015

@brettporter Thanks!

landed on v1.2.0

@lock
Copy link

lock bot commented Sep 14, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants