net: Try to turn on HTTP pooling.

[pcwalton]

But I don't see any actual pooling going on in Wireshark. What am I
doing wrong?

r? @jdm
r? @seanmonstar

[seanmonstar]

It looks correct to me. Here's possible reasons it's not reused:

• Connections are to different (scheme, host, port) triples (of course).
• The server responds with a Connection: close header.
• The response isn't drained (read till EOF).

Turning on RUST_LOG=hyper would help show if there are the case. Specifically, a TRACE:hyper::client::pool occurs when the PooledStream is dropped, listing is_closed (connection header) and is_drained (read to EOF).

If they're false and true respectively, you should see a second trace reuse (scheme, host, port).

---

One situation needs to handled manually in this PR: if the pooled connection has been killed or timed out by the server, it will still be in the Pool and given out when another connection is requested. I imagine handling the error case when passing Request::with_connector would need to check for something like io::ErrorKind::NotConnected or ConnectionAborted (I don't know yet), and if so, try again with the connector, which will use a different stream.

I haven't thought of a good way to do that myself in hyper yet.

[jdm]

I'm not keen on the huge block in which the connector is exclusively locked. Is there a way to minimize that?

[pcwalton]

@jdm I tried to get it as small as I reasonably could, but I could try to do better. Note that apparently if we do another hyperup, we won't need the lock at all.

[seanmonstar]

Yea, that will be in 0.5, no longer taking a &mut Connector: hyperium/hyper@1b31872

[pcwalton]

I minimized the lock and added retrying in the case @seanmonstar requested. r? @jdm

[jdm]

-S-awaiting-review +S-awaiting-answer

---

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

• components/net/http_loader.rs @ r1
• components/net/resource_task.rs @ r1

---

^{components/net/http_loader.rs, line 139 [r1] (raw file):}
Does this need to be mut? I'm also unclear why we can't use let writer = down below.

---

Comments from the review on Reviewable.io

[pcwalton]

Addressed comment. r? @jdm

[jdm]

@bors-servo: r+
-S-awaiting-review +S-awaiting-merge

---

Review status: all files reviewed, all discussions resolved, some commit checks pending.
Reviewed files:

• components/net/http_loader.rs @ r2

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 50a51b1 has been approved by jdm

[bors-servo]

⌛ Testing commit 50a51b1 with merge d04ca97...

[bors-servo]

💔 Test failed - mac1

[pcwalton]

Unexpected Results
==================

/old-tests/submission/Opera/script_scheduling/087.html
------------------------------------------------------
FAIL  scheduler: multiple defer scripts, one slow loading
/old-tests/submission/Opera/script_scheduling/105.html
------------------------------------------------------
FAIL  scheduler: adding async attribute at runtime
/old-tests/submission/Opera/script_scheduling/123.html
------------------------------------------------------
FAIL scheduler: altering the type attribute and adding/removing external script with async=false 
/old-tests/submission/Opera/script_scheduling/126.html
------------------------------------------------------
FAIL scheduler: altering the type attribute and changing script data external script async=false 

[bors-servo]

☔ The latest upstream changes (presumably #6125) made this pull request unmergeable. Please resolve the merge conflicts.

[seanmonstar]

I found eventually that in cases where the response was chunked, the stream wouldn't be reused. Responses with Content-Length would be. This has been fixed in hyper v0.6, though it has some breaking changes around using Ssl. They're not to difficult to change, just providing a OpenSSL SslContext instead of a ContextVerifier.

[metajack]

@pcwalton ping. This one is getting quite old.