Repository navigation
Set socket connecting state to always be false - #1182
Conversation
|
Hi @griffinmyers thanks for the pull request :) I’m catching up after a leave, I can’t promise when I get the time to look into it, but I’m very open to improve the implementation of the socket mocking. I’m not very familiar with it, so if you would be okay with helping out in case things break around it in future, I’d be happy to move forward with whatever you suggest, as long as current tests pass |
|
When you rebase on latest master then the SSL-related errors should resolve |
fc9b836 to
2e0dfe2
Compare
|
Awesome, thanks @gr2m -- just pushed an update. Looks like this change fixes up 41 tests that break on Happy to lurk and pitch in with any future socket issues. |
|
🎉 This PR is included in version 9.6.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
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! |
Background
I recently encountered the
socketDelaybug noted in #1174 (at least, fairly sure). More specifically, I observed breakage running my test suite on node versions>=10.7.0.nodejs/node#21204 looks like an extremely likely upstream cause of the behavior I'm experiencing with nock. It was released in node
v10.7.0so the timing fits as well.Notably, before that change, if a request's socket attribute was truthy and writeable,
req.setTimeoutwould invokesocket.setTimeoutwithout inspecting any connection state, likesocket.connecting(ref). Now,req.setTimeoutalways checks the current state of the socket (ref):'connect'event.I'm not super well versed in nock's guiding philosophy, but poking around the code it didn't seem like the mock socket implementation was particularly full featured or attempted to accurately model real socket behavior. For instance,
socket.connectingis set totruehere, but is never set tofalse. Further, the only time it looks like the'connect'event will be emitted is as a side-effect of registering the'socket'event onreq(ref), which is a bit weird (and fails to setsocket.connectingtofalseeven though it ought to be given the docs)Suggested Fix
Because the life-cycle of a socket isn't really modeled in nock, my suggested fix is to simply pretend as if the socket is already connected at the time of issuing the request. At least, this fixes the issues I'm experiencing.
A few tests appear to be broken on
master, but this patch doesn't appear to introduce any testing regressions, and fixes a handful in fact. There looks to a cert expiration issue effecting some of the tests, I'll post back with any thoughts as I dig a bit more into it.Happy to discuss alternative ideas!
edit: looks like the CA expiration was already tackled in https://github.com/nock/nock/pull/1181/files !
fixes #1174