Fix idle timeout rescheduling - #15656
Conversation
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
| CountDownLatch resetLatch = new CountDownLatch(1); | ||
| HeadersFrame frame = new HeadersFrame(newRequest("GET", HttpFields.EMPTY), null, false); | ||
| FuturePromise<Stream> promise = new FuturePromise<>(); | ||
| client.newStream(frame, promise, new Stream.Listener() |
There was a problem hiding this comment.
Use client.newStream(frame, new Stream.Listener()...); for simplicity.
| @Override | ||
| public boolean handle(Request request, Response response, Callback callback) | ||
| { | ||
| // Add a listener to force notIdle to run in a separate thread after cycle timeout rescheduling. |
There was a problem hiding this comment.
The comment is unclear: how does adding a timeout listener forces notIdle() to run in a separate thread, which notIdle() does it refer to, etc.
| h2.setStreamIdleTimeout(idleTimeout); | ||
| prepareServer(h2); | ||
| connector.setIdleTimeout(idleTimeout); |
There was a problem hiding this comment.
I would not set the stream idle timeout to be the same as the session idle timeout, it is just asking for races and flakyness.
If it is the stream that idle times out, just set the stream idle timeout, and force the session idle timeout to be larger, e.g. 10x.
| Stream stream = promise.get(5, TimeUnit.SECONDS); | ||
| ByteBuffer data = ByteBuffer.allocate(1024); | ||
| stream.data(new DataFrame(stream.getId(), data, true), Callback.NOOP); | ||
| assertTrue(resetLatch.await(3 * idleTimeout, TimeUnit.MILLISECONDS)); |
There was a problem hiding this comment.
It is not clear how the test receives a RST_STREAM, given that the idle timeout listener returns false every time, therefore ignoring it.
It is because the callback is not completed?
The test is difficult to interpret, and it is not evident the connection between the test setup and the assertions later.
| { | ||
| try | ||
| { | ||
| Thread.sleep(idleTimeout / 4); |
There was a problem hiding this comment.
Can you not use the utility sleep() method here?
3d594ea to
80923f7
Compare
…ld not send a reset frame only because the timeout listener returns true - rework HttpStreamOverHTTP2 channel recycling to use 3 states: can the channel be recycled, ca it not be recycled and are we past the recycling point as timeout and failures must mark the channel as non-recyclable but must continue to notify the channel until the recycling point has been reached - make sure channels are not recycled when Connection.isRecycleHttpChannels() is false Signed-off-by: Ludovic Orban <lorban@bitronix.be>
80923f7 to
c963d5d
Compare
2 participants
Fixes #15386