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.

Sign up for GitHub

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

Jump to bottom

Per request timeout #643

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Per request timeout #643

merged 5 commits into from
Apr 10, 2024

Conversation

hamnis
Copy link
Collaborator

@hamnis hamnis commented Mar 21, 2024

No description provided.

@hamnis
Add per request timeout
b08ee2e 
Store the current in flight callback in a variable.

When the timeout handler reaches timeout, we kill the current request.

If the idle timeout triggers, we kill all requests, including the current in-flight request.
@hamnis
Make sure we start the next request
e907127 
ensure we call ctx.read() when killing current request to start the next request
@hamnis
use logger instead of println
4d0b094
@hamnis
read only if autoread is off
7a5ce4a
@mkharytonau
Copy link

Hmm, client/testOnly org.http4s.netty.client.NettyClientIdleTimeoutTest passing while client/testOnly org.http4s.netty.client.NettyClientIdleTimeoutTest -- "*Request A timed out, idle timeout kills connection" not (request B still receives response A).

@mkharytonau
Copy link

mkharytonau commented Mar 22, 2024 •
edited
Loading

Regarding Request A timed out, request B receives response B, won't withReadTimeout interrupt client call? As I see, the 2nd client call still waits until server responses to the 1st call meaning 1st client call still hangs until server response. Meaning setting case GET -> Root / "1" => respond("1", 105.seconds, "1") will just hang this test.

You see, client logs message, but not being interrupted:

09:42:05.249 [io-compute-5] TRACE o.h.n.c.NettyClientIdleTimeoutTest - server: received /1 request, sleeping... 
09:42:08.202 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:11.201 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:14.202 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:17.203 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:20.204 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:23.205 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:26.206 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:29.207 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:32.208 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read

@hamnis
Copy link
Collaborator Author

hamnis commented Mar 22, 2024

Regarding Request A timed out, request B receives response B, won't withReadTimeout interrupt client call? As I see, the 2nd client call still waits until server responses to the 1st call meaning 1st client call still hangs until server response. Meaning setting case GET -> Root / "1" => respond("1", 105.seconds, "1") will just hang this test.

You see, client logs message, but not being interrupted:

09:42:05.249 [io-compute-5] TRACE o.h.n.c.NettyClientIdleTimeoutTest - server: received /1 request, sleeping... 
09:42:08.202 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:11.201 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:14.202 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:17.203 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:20.204 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:23.205 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:26.206 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:29.207 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read 
09:42:32.208 [KQueueEventLoopGroup-3-2] TRACE o.h.n.c.Http4sHandler - Timing out request due to missing read

yes, this is wrong. we should get the next pending promise instead.
There is another underlying issue here. The pipeline contains a handler which has its own queue, and this screws up the ordering of requests.

Until this is resolved I think adding a middleware for handling per request timeout is the best for now.

@hamnis
Copy link
Collaborator Author

hamnis commented Apr 10, 2024

Will have to come back to this with a new solution, closing for now

@hamnis hamnis closed this Apr 10, 2024
@hamnis
Always kill connection when failing either idle timeout
0258df3 
We control when we kill the connection, not the playframework lib.
@hamnis hamnis reopened this Apr 10, 2024
@hamnis hamnis merged commit 4d9a14c into series/0.5 Apr 10, 2024
14 checks passed
@hamnis hamnis deleted the per-request-timeout branch April 10, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hamnis @mkharytonau