Use upstream net-http-persistent, connection_pool (un-vendored) - #345
Conversation
|
|
nevans
changed the title
net-http-persistent and connection_poolnet-http-persistent and connection_pool (un-vendored)
nevans
changed the title
net-http-persistent and connection_pool (un-vendored)net-http-persistent, connection_pool (un-vendored)
|
@chrisarcand @brandonc Sorry to ping you on this. I just noticed that Do you have any feedback on the approach I've used here? Is there some reason the |
|
Ah thanks for the ping. We're in a period of ownership transition here at IBM/HashiCorp so indeed, sorry for the no response here, this definitely looks like it needs doing. I'll get back to this by early next week and if you don't hear from me then, feel free to ping several times 😄 |
|
@chrisarcand ping 😉 Thanks for the update. Given that this is Thanksgiving week in the USA, you might not have time for it this week. But, you asked for a reminder so here it is! 🙂 For what it's worth, I've been running (in production) a branch that merges the following PRs:
I was originally running #329 because I was debugging an issue that was eventually fixed by this PR. The problem was that we had forking processes that connected in the parent process but didn't disconnect/reconnect in the child processes. The latest version of The errors we got from the sharing connection between forked processes were that one process would read the responses to requests made by another process. Most of the time, that resulted in protocol errors, parse errors, or nil reference errors. But, as you can imagine, those weren't the only results. It could've been truly catastrophic (and when we first discovered the bug, before we put safeguards in place, it nearly was). So, IMO, this is a very high priority PR. And yes, with my experience, I should've known (without being told) when we first added vault to our forking codebase that we'd need to handle disconnects on fork! But IMO, this should also be in the README, either as "You need to reconnect on fork" or as "This gem automatically reconnects on fork". (I can make a separate PR for that! 😉) |
|
Thanks! Indeed I sorta forgot I was taking the whole week off for the US holiday ;) I'm going to start with the base (#346) and then move to this - this looks real good though. Securing some checkboxes to be able to get this stuff merged, thanks for your patience. |
0139d86 to
85abe54
Compare
|
|
||
| @nhp = PersistentHTTP.new("vault-ruby", nil, pool_size, pool_timeout) | ||
| @nhp = Net::HTTP::Persistent.new(name: "vault-ruby", pool_size:) | ||
| @nhp.pool.instance_variable_set(:@timeout, pool_timeout) |
There was a problem hiding this comment.
This is definitely a little Hacky but but reasonable given that it doesn't appear there's any upstream method to set this.
There was a problem hiding this comment.
Yeah, that really annoyed me too. But the main risk from this is timeouts not working correctly, and (IMO) that's still safer than not being able to get fixes from upstream.
|
Sigh. The merge commit screwed things up, I'll rebase this. |
`net-http-persistent` and `connection_pool` were vendored, and have gotten very out-of-date. This resets to use the upstream gems. `connection-pool` is upgraded from 2.2.0 to 2.4+: * 2.2.5 fixes argument forwarding for ruby 2.7+. * 2.4.0 automatically drops all connections after fork. `net-http-persistent` is upgraded from 3.0.0 (forked) to 4.0.2+: * 4.0.2 fixes compatibility with `connection-pool` 2.4 * 3.1.0 supports TLS min/max and IPv6 (also in vendored fork) * 3.1.0 fixes a memory leak in connection pooling. * some vendored fork differences were removed: * use :name, :pool_size kwargs for .new One change to the vendored fork was reimplemented: upstream does _not_ support timeout kwarg for .new, nor does `connection_pool` expose an accessor for its `timeout`. So this code reaches into the pool and sets the `@timeout` ivar directly.
These exceptions are copied from the list of exceptions which Net::HTTP uses in `#transport_request` to determin whether the socket should be closed (and retried, for idempotent requests).
e057ca6 to
81bf1d0
Compare
3 participants
net-http-persistentandconnection_poolwere vendored, and have gotten very out-of-date. This resets to use the upstream gems.connection-poolis upgraded from 2.2.0 to 2.4+:net-http-persistentis upgraded from 3.0.0 (forked) to 4.0.2+:connection-pool2.4One change to the vendored fork was reimplemented: upstream does not support timeout kwarg for .new, nor does
connection_poolexpose an accessor for itstimeout. So this code reaches into the pool and sets the@timeoutivar directly.Additionally, all of the exceptions which
net-httphandles (inNet::HTTP#transport_request) are added toRESCUED_EXCEPTIONS. Net::HTTP uses these to determine whether the socket should be closed (and whether idempotent requests should be retried).Please note: this incorporates #346, so CI can pass.