-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Revert futures 0.2 changes #1482
Conversation
From the CI errors, it looks like we might need to back-port some of the fixes I included for the new tokio behaviour. I'll see if I can figure out which ones they are! |
bc3b8e4
to
7d5e634
Compare
On my local machine, the test |
Yeah, it‘s a race condition I fixed as part of the futures 0.2 changes
that are being reverted here, should be a matter of backporting it!
…On Tue, Apr 10, 2018, at 5:43 PM, Kam Y. Tse wrote:
On my local machine, the test `conn_drop_prevents_pool_checkout` can
pass some time.
Seems there is a race somewhere, I am trying locate the problem.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#1482 (comment)
|
tests/client.rs
Outdated
@@ -1043,27 +1042,28 @@ mod dispatch_impl { | |||
let mut buf = [0; 4096]; | |||
sock.read(&mut buf).expect("read 1"); | |||
sock.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n").unwrap(); | |||
//sock.read(&mut buf).expect("read 2"); | |||
sock.read(&mut buf).expect("read 2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is what made it pass always :D
4f5ce94
to
250b238
Compare
This reverts commit a12f7be. Much sadness 😢.
250b238
to
c210524
Compare
I want to know why removing that test case. |
@kamyuentse I kept trying variants, and couldn't seem to get it passing 100% of the time without disabling the exact thing it was testing. And so then I read through it, trying to remember what the heck it was even testing, and realized it was because before inside Actually, I'm fairly certain the problem is the race condition in futures mpsc queue: rust-lang/futures-rs#909 That'd make sense, since the test was basically dropping the |
@seanmonstar Thanks for your explanation! I don't know whether futures issue cause tokio worker panic, but I think I should cc @carllerche . |
See #1481
😢