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

net: Try to turn on HTTP pooling. #6948

Closed
wants to merge 1 commit into from
Closed

Conversation

@seanmonstar
Copy link
Contributor

seanmonstar commented Aug 4, 2015

rebased and fixed up #6026

(Can't compile some of the native dependencies on my current machine, so I'll have to rely on travis?).

Also, better if viewed with w=1.

Review on Reviewable

@jdm
Copy link
Member

jdm commented Aug 4, 2015

Exciting! Thanks for getting this ball rolling again :)
-S-awaiting-review +S-needs-code-changes


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


components/net/http_loader.rs, line 40 [r1] (raw file):
Rebase error?


components/net/resource_task.rs, line 27 [r1] (raw file):
nit: move this below hyper::mime.


Comments from the review on Reviewable.io

@seanmonstar
Copy link
Contributor Author

seanmonstar commented Aug 4, 2015

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


components/net/http_loader.rs, line 40 [r1] (raw file):
Indeed. Fixed.


Comments from the review on Reviewable.io

@seanmonstar
Copy link
Contributor Author

seanmonstar commented Aug 4, 2015

Oh dang, travis only does make tidy? I thought it ran the test suite. I don't currently have a way to make sure this compiles...

@jdm
Copy link
Member

jdm commented Aug 4, 2015

Ok, I'll pull locally.

@seanmonstar seanmonstar force-pushed the seanmonstar:pool branch 2 times, most recently from 548d919 to 7134f33 Aug 5, 2015
@seanmonstar
Copy link
Contributor Author

seanmonstar commented Aug 5, 2015

It builds! And is a bit different.

Side note: if the --no-ssl option could become a compile time option, such that env!(NO_SSL) could be used, then the SslProvider wouldn't need to be an enum, which would prevent the branch in wrap_client.

@jdm jdm self-assigned this Aug 5, 2015
@jdm
Copy link
Member

jdm commented Aug 5, 2015

Almost there!
-S-awaiting-review +S-needs-code-changes


Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/net/Cargo.toml, line 45 [r2] (raw file):
This was a mistake I made; we can remove it.


components/net/http_loader.rs, line 199 [r2] (raw file):
This block is no longer necessary afaict; let's unindent everything below it.


components/net/lib.rs, line 24 [r2] (raw file):
Remove this too.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Aug 6, 2015

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


Reviewed 3 of 3 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Aug 6, 2015

📌 Commit 6d7d6d5 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 6, 2015

Testing commit 6d7d6d5 with merge f8a9472...

bors-servo pushed a commit that referenced this pull request Aug 6, 2015
net: Try to turn on HTTP pooling.

rebased and fixed up #6026 

(Can't compile some of the native dependencies on my current machine, so I'll have to rely on travis?).

Also, better if viewed with [w=1](https://github.com/servo/servo/compare/servo:master...seanmonstar:pool?expand=1&w=1).

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

bors-servo commented Aug 6, 2015

💔 Test failed - linux2

@jdm
Copy link
Member

jdm commented Aug 6, 2015

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

Can you see if this fails locally for you?

@jdm jdm added S-tests-failed and removed S-awaiting-merge labels Aug 6, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2015

Testing commit f31f734 with merge cd3a5e2...

bors-servo pushed a commit that referenced this pull request Aug 24, 2015
net: Try to turn on HTTP pooling.

rebased and fixed up #6026 

(Can't compile some of the native dependencies on my current machine, so I'll have to rely on travis?).

Also, better if viewed with [w=1](https://github.com/servo/servo/compare/servo:master...seanmonstar:pool?expand=1&w=1).

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

bors-servo commented Aug 24, 2015

💔 Test failed - linux2

@seanmonstar seanmonstar force-pushed the seanmonstar:pool branch from f31f734 to 4c1eec7 Aug 24, 2015
@seanmonstar
Copy link
Contributor Author

seanmonstar commented Aug 24, 2015

Updated tests, marking many that expected TIMEOUT as PASS.

However, a few that were FAIL are now TIMEOUT. I have no way of locally running the wpt tests.

@jdm
Copy link
Member

jdm commented Aug 24, 2015

I'll take a look at them.

@bors-servo
Copy link
Contributor

bors-servo commented Aug 26, 2015

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

@jdm
Copy link
Member

jdm commented Aug 26, 2015

Fascinating!

 0:05.12 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16557) Full command: /Users/jdm/src/servo/target/debug/servo --cpu --hard-fail -u Servo/wptrunner http://localhost:8000/old-tests/submission/Opera/script_scheduling/085.html --user-stylesheet /Users/jdm/src/servo/resources/ahem.css
(pid:16557) "thread 'http_loader' panicked at 'Http11Message lost its underlying stream somehow', /Users/jdm/.cargo/registry/src/github.com-0a35038f75765ae4/hyper-0.6.10/src/http/h1.rs:270"
@jdm
Copy link
Member

jdm commented Aug 26, 2015

(pid:16784) "thread 'http_loader' panicked at 'Http11Message lost its underlying stream somehow', /Users/jdm/.cargo/registry/src/github.com-0a35038f75765ae4/hyper-0.6.10/src/http/h1.rs:270"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "stack backtrace:"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   1:        0x10d286b00 - sys::backtrace::write::h095d0f07f92e7e3eYts"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   2:        0x10d289ed3 - panicking::on_panic::h5f4115f30a6b4860Z7w"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   3:        0x10d278e92 - rt::unwind::begin_unwind_inner::h7c54671eea5e313e0Cw"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   4:        0x10cf12c17 - rt::unwind::begin_unwind::h15331884341034434895"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   5:        0x10cf73f53 - http::h1::Http11Message::get_mut::h89ef6e1f04f35dcfBGj"
 0:04.83 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   6:        0x10ace5ba1 - http::h1::Http11Message.HttpMessage::close_connection::he6540d19a3caa4cfQDj"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   7:        0x10cf320d1 - client::response::Response::with_message::h0a1bf58d6ed303adlSa"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   8:        0x10cf31b5d - client::request::Request<Streaming>::send::hf31638ee583678caqNa"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "   9:        0x10acc8bbb - http_loader::load::h23f3aea7c64866a5dpa"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  10:        0x10acc4187 - http_loader::factory::closure.26120"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  11:        0x10acc3f1f - task::spawn_named::closure.26113"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  12:        0x10acc3e77 - boxed::F.FnBox<A>::call_box::h8285890207617347537"
 0:04.84 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  13:        0x10acb311b - boxed::Box<FnBox<A, Output $u3d$$u20$R$GT$$u2b$$u20$Send$u20$$u2b$$u20$$u27$a$GT$.FnOnce$LT$A$GT$::call_once::h4130720213201258293"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  14:        0x10acb2cfd - thread::Builder::spawn_inner::closure.25099"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  15:        0x10acb2ca9 - rt::unwind::try::try_fn::h16787200724282224943"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  16:        0x10d2898a8 - __rust_try"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  17:        0x10d285500 - rt::unwind::try::inner_try::h39d984f71e9cc780Tyw"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  18:        0x10acb2c13 - rt::unwind::try::h17106555173943191013"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  19:        0x10acb2a8b - thread::Builder::spawn_inner::closure.25075"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  20:        0x10acb337c - boxed::F.FnBox<A>::call_box::h15713970663752920389"
 0:04.85 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  21:        0x10d288b9d - sys::thread::Thread::new::thread_start::h999b3790b6d4b23deXv"
 0:04.86 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  22:     0x7fff928ae898 - _pthread_body"
 0:04.86 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:16784) "  23:     0x7fff928ae729 - _pthread_start"
@seanmonstar seanmonstar force-pushed the seanmonstar:pool branch from 4c1eec7 to 8de6def Aug 27, 2015
@seanmonstar
Copy link
Contributor Author

seanmonstar commented Aug 27, 2015

@jdm boo, panics. I've updated to hyper 0.6.11, which fixes those panics (and some other possible ones). I'm not entirely sure which tests that I've marked as TIMEOUT instead of FAIL should be reverted...

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.
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

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

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 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 -->
@jdm
Copy link
Member

jdm commented Sep 1, 2015

Closing this in favour of #7418.

@jdm jdm closed this Sep 1, 2015
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 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 -->
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

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