Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable HTTP connection pooling #7418

Merged
merged 1 commit into from Sep 2, 2015
Merged

Enable HTTP connection pooling #7418

merged 1 commit into from Sep 2, 2015

Conversation

@jdm
Copy link
Member

jdm commented Aug 27, 2015

Rebased and adjusted version of #6948. Closes #6948.

Review on Reviewable

@jdm
Copy link
Member Author

jdm commented Aug 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Aug 27, 2015

📌 Commit 6f24e66 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 27, 2015

Testing commit 6f24e66 with merge adfdf78...

bors-servo pushed a commit that referenced this pull request Aug 27, 2015
Enable HTTP connection pooling

Rebased and adjusted version of #6948. Closes #6948.
@jdm
Copy link
Member Author

jdm commented Aug 27, 2015

This is going to fail because I forgot to update XMLHttpRequest/response-method.htm.ini . That being said, the tests that are now passing make me suspect that something is weird in our non-pooling code, because I can't figure out why they would fail.

@jdm
Copy link
Member Author

jdm commented Aug 27, 2015

@jdm jdm force-pushed the jdm:httppool branch from 6f24e66 to c254ad0 Aug 28, 2015
@jdm
Copy link
Member Author

jdm commented Aug 28, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Aug 28, 2015

📌 Commit c254ad0 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 28, 2015

Testing commit c254ad0 with merge ed7624f...

bors-servo pushed a commit that referenced this pull request Aug 28, 2015
Enable HTTP connection pooling

Rebased and adjusted version of #6948. Closes #6948.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7418)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 28, 2015

💔 Test failed - linux2

@jdm
Copy link
Member Author

jdm commented Aug 28, 2015

/css21_dev/html4/background-color-075.htm
-----------------------------------------
TIMEOUT [Parent]
 1:02.98 TEST_START: Thread-TestrunnerManager-35 /css21_dev/html4/background-color-075.htm
 1:03.19 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) Full command: /home/servo/buildbot/slave/linux2/build/target/release/servo --cpu --hard-fail --exit -u Servo/wptrunner -Z disable-text-aa --output=/tmp/tmpRp06Iu/a6fe72d9-a29b-40e3-88c5-e4dac2995b9f http://localhost:8000/css21_dev/html4/background-color-075.htm --user-stylesheet /home/servo/buildbot/slave/linux2/build/resources/ahem.css
(pid:27586) "thread 'http_loader' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/libcore/result.rs:732"
 1:03.19 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "stack backtrace:"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   1:     0x7f645f4335c9 - sys::backtrace::write::h29675241e432c301Pws"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   2:     0x7f645f436eb9 - panicking::on_panic::h1e1c334783ff2a38jzx"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   3:     0x7f645f4247fe - rt::unwind::begin_unwind_inner::hb529041d76c2c802L1w"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   4:     0x7f645f425111 - rt::unwind::begin_unwind_fmt::h027d2621615b5dcfR0w"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   5:     0x7f645f4368b1 - rust_begin_unwind"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   6:     0x7f645f466a7f - panicking::panic_fmt::ha8483564b9ba8c9c47D"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   7:     0x7f645e8425c9 - http_loader::load::h19ac6a5bc624e6cfdpa"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   8:     0x7f645e83d504 - boxed::F.FnBox<A>::call_box::h11481485006043150976"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "   9:     0x7f645e837244 - rt::unwind::try::try_fn::h12677946051234551213"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  10:     0x7f645f436858 - __rust_try"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  11:     0x7f645f4314c2 - rt::unwind::try::inner_try::hbf179233f048e0c1EXw"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  12:     0x7f645e8373de - boxed::F.FnBox<A>::call_box::h3081112056388942627"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  13:     0x7f645f435c43 - sys::thread::Thread::new::thread_start::hb07a6549e7b84f0f46v"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  14:     0x7f645cdca181 - start_thread"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  15:     0x7f645d8ff47c - __clone"
 1:03.47 PROCESS_OUTPUT: Thread-TestrunnerManager-35 (pid:27586) "  16:                0x0 - <unknown>"
@seanmonstar
Copy link
Contributor

seanmonstar commented Aug 28, 2015

@jdm there's several unwraps in that function :(

@seanmonstar
Copy link
Contributor

seanmonstar commented Aug 28, 2015

99:        Ok(p) => p.send(Done(Err(err))).unwrap(),
156:        let inner_url = load_data.url.non_relative_scheme_data().unwrap();
157:        doc_url = Url::parse(inner_url).unwrap();
173:        if &*url.scheme != "https" && request_must_be_secured(&hsts_list.lock().unwrap(), &url) {
218:                    let load_data = LoadData::new(Url::from_file_path(&*image).unwrap(), None);
231:                hostname: doc_url.serialize_host().unwrap(),
260:            let (tx, rx) = ipc::channel().unwrap();
263:                                                                CookieSource::HTTP)).unwrap();
265:            if let Some(cookie_list) = rx.recv().unwrap() {
325:                                                                 net_event))).unwrap();
357:                                                                        CookieSource::HTTP)).unwrap();
379:                    ).unwrap();
469:                                                             net_event_response))).unwrap();

These are all the unwraps that could happen in the load function. They're either:

  • Url serialization
  • ipc channel creation
  • a mutex lock of hsts_list
  • channel send() and recv().
@jdm
Copy link
Member Author

jdm commented Aug 28, 2015

Unless we happen to be calling something that's inlined and unwrapping :/

@seanmonstar
Copy link
Contributor

seanmonstar commented Aug 28, 2015

Took a while for me to realize, but its a Result<_, ()>, so what uses () as an error?

@seanmonstar
Copy link
Contributor

seanmonstar commented Aug 28, 2015

Hyper doesn't ever use that as an error type, and I don't think Url does either. Mutex doesn't. Looking at ipc-channel, I see its the error type of both channel creation and sending and recving.

@bors-servo
Copy link
Contributor

bors-servo commented Aug 28, 2015

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

@bors-servo
Copy link
Contributor

bors-servo commented Sep 2, 2015

Testing commit ee8f96b with merge 580829d...

bors-servo pushed a commit that referenced this pull request Sep 2, 2015
Enable HTTP connection pooling

Rebased and adjusted version of #6948. Closes #6948.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7418)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 2, 2015

💔 Test failed - mac-dev-ref-unit

@jdm
Copy link
Member Author

jdm commented Sep 2, 2015

/old-tests/submission/Opera/script_scheduling/087.html
------------------------------------------------------
FAIL  scheduler: multiple defer scripts, one slow loading

It's almost like this is complicated stuff that's tickling complicated tests!

@seanmonstar
Copy link
Contributor

seanmonstar commented Sep 2, 2015

I feel like we went in a big circle: that's the original failing test of this PR!

@seanmonstar
Copy link
Contributor

seanmonstar commented Sep 2, 2015

(pid:39073) "ERROR:script::dom::htmlscriptelement: error loading script os error"
@bors-servo
Copy link
Contributor

bors-servo commented Sep 2, 2015

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

@jdm
Copy link
Member Author

jdm commented Sep 2, 2015

@jdm
Copy link
Member Author

jdm commented Sep 2, 2015

I assume the problem comes from

 0:03.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "INFO:net::http_loader: got HTTP response 200 OK, headers:"
 0:03.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "INFO:net::http_loader:  - Date: Wed, 02 Sep 2015 14:20:50 GMT"
 0:03.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "INFO:net::http_loader:  - Content-Type: text/javascript"
 0:03.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "INFO:net::http_loader:  - Server: BaseHTTP/0.3 Python/2.7.9"
 0:05.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "TRACE:hyper::http::h1: eofread: Ok(26)"
 0:05.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "TRACE:hyper::http::h1: eofread: Ok(0)"
 0:05.87 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:36440) "TRACE:hyper::client::response: Response.drop is_drained=true"

since include-1.js is coming through a delayed pipe in the test.

@jdm jdm force-pushed the jdm:httppool branch from ee8f96b to 6395ccf Sep 2, 2015
@seanmonstar
Copy link
Contributor

seanmonstar commented Sep 2, 2015

Ah, the shame. I didn't notice that EofReader by definition means that the connection will be closed at the end. I've published v0.6.13 that should properly handle this.

@jdm jdm removed the S-needs-rebase label Sep 2, 2015
@jdm jdm force-pushed the jdm:httppool branch from 6395ccf to a1a9db8 Sep 2, 2015
@jdm
Copy link
Member Author

jdm commented Sep 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 2, 2015

📌 Commit a1a9db8 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 2, 2015

Testing commit a1a9db8 with merge 366d4a8...

bors-servo pushed a commit that referenced this pull request Sep 2, 2015
bors-servo
Enable HTTP connection pooling

Rebased and adjusted version of #6948. Closes #6948.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7418)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 2, 2015

@bors-servo bors-servo merged commit a1a9db8 into servo:master Sep 2, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.