[BREAKING]: Remove client support for pipelined requests #783
Conversation
Follows up on the discussion and work started in #732. The gist is this: supporting "pipelined requests" is not well-supported across the web these days and severely complicates the threadsafe client implementation in HTTP.jl. And as has been pointed out in various discussions, the attempt to support pipelining is affecting our ability to avoid thread-safety issues in general (see #517). This commit has a few key pieces: * Splits "connection pooling" logic into a new connectionpools.jl file * Removes pipeline-related fields/concepts from the ConnectionPool.jl file and specifically the `Connection`/`Transaction` data structures * Attempts to simplify a lot of the work/logic around managing a `Transaction`'s lifecycle Things I haven't done yet: * Actually deprecated the `pipeline_limit` keyword argument * Got all tests passing; I think the websockets/server code isn't quite ironed out yet, but I'll dig into it some more tomorrow Big thanks to @nickrobinson251 for help reviewing the connectionpools.jl logic. Pinging people to help review: @c42f, @vtjnash, @s2maki, @fredrikekre
|
Pushed some more updates; notable I went ahead and removed the More tests are passing, but there are still some failures in websocksets and servers, so I'll move on to those next. |
Codecov Report
@@ Coverage Diff @@
## master #783 +/- ##
==========================================
- Coverage 77.43% 77.25% -0.19%
==========================================
Files 38 39 +1
Lines 2562 2493 -69
==========================================
- Hits 1984 1926 -58
+ Misses 578 567 -11
Continue to review full report at Codecov.
|
|
Ok, made some good progress today; tests are passing everywhere except windows, so I'll need to dig my windows machine out, blow the dust off, and try to figure out what's going on there. My guess is a slight change in behavior about when connections get closed and how long they stay valid; this comes from finding several issues where connection/transaction lifetimes weren't really being managed properly on current master code. I had the idea of setting up a google/zoom video call on Friday for anyone interested in doing a thorough walk-through of the changes here; would people be interested in that? @fredrikekre, @christopher-dG, @c42f, @oxinabox, or anyone else? |
|
sure, keen. |
There was a problem hiding this comment.
This seems great!
The concurrent aspects are an enormous amount clearer now, and I think the pool now has a chance of being correct (it's still a bit subtle though, I wouldn't want to bet on it either way 😆)
I've done a thorough review, having looked at this code before. Major points (discussed in depth in inline comments):
- I think we could simplify the acquire/release further by always passing connections via the buffer of idle connections
- It seems fundamentally buggy to use
hashconnto identify connections via the hash rather than the actual connection key fields. What protects us from hash collisions and sending data to the wrong host? - I think
sslupgradecan forget tocloseconnections as it acts weirdly outside the cache with an existing connection. - Can we drop
reuse_limitcompletely? IIUC curl doesn't have this kind of thing and maybe we don't need it for HTTP clients.
| get!(() -> Pod(), pool.conns, x) | ||
| end | ||
| end | ||
| const POOL = Pool(Connection) |
There was a problem hiding this comment.
A side comment (not todo in this PR) — I'd love it if the pool was attached to the HTTP stack instead of being a magic global shared by all stacks behind the scenes. Then people using their own stack wouldn't need global state at all.
(Of course, this means the HTTP stack would need to be a value rather than a type, which is breaking.)
There was a problem hiding this comment.
I think moving the layer machinery to be based on values instead of types would be good as well. We should make the change for 1.0
* Added `GC.@preserve` annotations * Removed calls to `hash` in `hashconn` and renamed to `connectionkey` * Removed enforcement of `reuse_limit` * Renamed some fields/variables for clarity * Cleaned up comments for clarification in connectionpools.jl * Added an additional `acquire` method that accepts an already created connection object for `Pod` insertion/tracking
|
Ok, just pushed a commit that covers everything @c42f brought up in review. |
|
Tests are all passing (except the known failing windows case that I'll look into next #787 ). Does anyone else want to review? Happy to wait. I'm also still happy to setup a walk-through call if anyone is interested and can give me a date and time that would work for them. My schedule is pretty flexible this week, so basically any time. |
|
If no other comments/review arise, I plan on merging this in about 12 hours. Then we can push on other 1.0 issues to try and get that out soonish as to not keep the master branch in too weird of a non-released state. |
quinnj
changed the title
Follows up on the discussion and work started in
#732.
The gist is this: supporting "pipelined requests" is not well-supported
across the web these days and severely complicates the threadsafe client
implementation in HTTP.jl. And as has been pointed out in various
discussions, the attempt to support pipelining is affecting our ability
to avoid thread-safety issues in general (see
#517).
This commit has a few key pieces:
file and specifically the
Connection/Transactiondata structuresTransaction's lifecycleThings I haven't done yet:
pipeline_limitkeyword argumentquite ironed out yet, but I'll dig into it some more tomorrow
Big thanks to @nickrobinson251 for help reviewing the connectionpools.jl
logic.
Pinging people to help review: @c42f, @vtjnash, @s2maki, @fredrikekre