Confirm concurrent HTTP/2 requests with empty flow-control window. #4127
Conversation
| // flow-control window. | ||
| response1.close(); | ||
|
|
||
| assertEquals("abc", response2.body().string()); |
There was a problem hiding this comment.
Can we make the test stronger e.g. assert nothing can be read until after response1.close? Perhaps using inputStream.available()?
There was a problem hiding this comment.
I'm not sure if there is a good way of checking that. It looks like available() will always return 0 unless you trigger a read. From my perspective, this test is proving that the second request will proceed as usual given the actions from the first request.
|
@dave-r12 this looks like it could be the issue I am seeing. My second request always fails when attempting to take the headers. For completeness' sake, do you have any pointers on where i could throw a breakpoint in the OKHTTP code to confirm this in my test app? |
|
@natez0r currently this test passes! So there is likely something different in your environment.
A possible first step: could you turn on frame logging and paste in the output? |
|
When I wrote that comment, I had checked out your patch and that test failed for me:
|
|
I ran my test app with frame logging, here are the logs frame_log.txt Socket timeout was thrown at 5:59:24 in the logs
|
I thought I had reproduced but realized I made a change to okhttp module for the test to pass (need to flush the HEADERS frame for the second request.) Can you compile/install that module (okhttp) and run again? |
That's helpful, thanks. It almost looks like there is a buffer sitting between the client/server and it's trickling DATA frames to the client. In other words, there's a backlog of DATA frames that are waiting to be delivered from the first request and the response to the second request is at the end of that backlog. What happens if you increase the read timeout of the second request? |
heh, I wonder if this is how genymotion throttles connections to make them 'Edge', which is not a great actual approximation of network speed. I had previously upped the timeout and still saw issues, but now i upped it to 500 seconds and can't get the socket timeout to happen again. |
after doing a |
| connection.writeSynReply(id, outFinished, responseHeaders); | ||
|
|
||
| if (outFinished) { | ||
| if (outFinished || connectionWindowEmpty) { |
There was a problem hiding this comment.
Tempted to refactor this so it’s more symmetric with the code in Http2Connection.newStream(). In particular:
- we could replace the
outFinishedlocal with a different boolean,flushHeaders. - we could only check if the connection window is empty if
flushHeadersis false.
But as-is this definitely makes responses and requests work more similarly and therefore I think it’s a big improvement.
There was a problem hiding this comment.
ie.
if (!flushHeaders) {
synchronized (connection) {
flushHeaders = connection.bytesLeftInWriteWindow == 0L;
}
}
There was a problem hiding this comment.
Turns out it isn’t quite that simple, cause we need to keep outFinished to send with the headers. Regardless, I did this fixup.
|
Nice improvement. |
swankjesse
force-pushed
the
concurrent-http2-flow-control
branch
from
8f86110
to
f5905f4
Compare
I'm not fully convinced this is worth merging. I just wanted to attempt to reproduce the steps in #3915.