-
Notifications
You must be signed in to change notification settings - Fork 544
Reintroduce timeout and keep-alive for watch requests to match client-go #2367
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Reintroduce timeout and keep-alive for watch requests to match client-go #2367
Conversation
|
Welcome @rossanthony! |
/easycla |
a1d444f
to
fe8d1c1
Compare
Thanks for finishing this up. I'll let @cjihrig have the final LGTM, but looks good to me. /approve |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, cjihrig, rossanthony The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@brendandburns or @cjihrig what's the process for getting a new release cut? |
Thanks to @Alabate for opening the initial PR with this fix: #2131
This PR includes Alabate's fix + unit test coverage. Below is the original PR description copied here for reference:
This will send keep-alive probes to the server every 30 seconds. These features were present prior to the 1.0 but were inadvertently removed.
Fixes #2127
Previous relevant issues:
Implementation details
Setting
requestInit.timeout = 30000
is equivalent tosocket.setTimeout(30000)
as you can see in the sources.One of the solution suggested in #2127 was to use the keepAlive of the
http.Agent
. But as documented here, that's just a boolean that instruct the agent that we want sockets to be re-used. We want to send packets at a fixed rate to know when a connection is broken. This can be done with the socket.setKeepAlive() method, but to use it, we need to access the socket object.I managed to access the socket and call
setKeepAlive()
, but only after theawait fetch()
, when the response is already arriving. That's the only way I found to access the socket. The Agent create the socket, but there is no event to know when a socket created, you can just access them by list like I did. There is a socket event on the request object, butnode-fetch
abstract it away, and I couldn't find a clean way to access it (node-fetch/node-fetch#1720 that request this feature).The only way I've found to get the socket before the request was by monkey-patching the
agent.createConnection()
, but I think this solution is worse.