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

[http] Mixed timeout / deadline on h1 dispatcher #1192

Open
joelwurtz opened this issue Jan 30, 2025 · 4 comments
Open

[http] Mixed timeout / deadline on h1 dispatcher #1192

joelwurtz opened this issue Jan 30, 2025 · 4 comments

Comments

@joelwurtz
Copy link
Contributor

joelwurtz commented Jan 30, 2025

It seems there is a bug when keep alive timeout is lower than tls accept timeout

By example using this :

let config = HttpServiceConfig::new()
   .keep_alive_timeout(Duration::from_millis(1000))
   .tls_accept_timeout(Duration::from_millis(4000))
;

Then doing some requests and shutdown the server, it will wait 4000ms (when connection is already open so there is no tls accept at this time)

Can be checked with current http tests, i'm not sure how the lazy deadline work so didn't do a PR but if you have some pointer can do it.

@fakeshadow
Copy link
Collaborator

It's more like a feature than bug.

In xitca-http timers are lazy to reduce memory allocation and syncing of runtime timer lock. Personally I would treat this as edge case that doesn't really make sense. tls accept timer in real should be have a smaller duration.

That said I'm open to add more syncing to the timer but let's wait and see if there are real world case where this issue matters to user. We really want to avoid locking the runtime every time the timer is polled.

@joelwurtz
Copy link
Contributor Author

Yeah, it just was strange that when trying to only set the keep alive timeout to a value lower than the default tls accept timeout it was hanging longer, so maybe the only change would be to warn user when the accept timeout is longer than keep alive / request head timeout

@fakeshadow
Copy link
Collaborator

I believe this issue can be used as an open question to others and we welcome further feedback on this topic. So I re-opened the issue and if @joelwurtz prefer it closed please do so and sorry for not asking beforehand.

@fakeshadow fakeshadow reopened this Feb 6, 2025
@joelwurtz
Copy link
Contributor Author

It was my mistake closing this, let's keep it open wand wait for feedback, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants