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

ref(server): make idle time of a http connection configurable #4248

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Nov 13, 2024

With the idle time configurable we can prevent a pile up of open connections which never see any activity.

See also on the hyper issue tracker:

https://github.com/hyperium/hyper/pull/3743
https://github.com/hyperium/hyper/issues/1628
https://github.com/hyperium/hyper/issues/2355
https://github.com/hyperium/hyper/pull/2827

@Dav1dde Dav1dde requested a review from a team as a code owner November 13, 2024 21:02
@Dav1dde Dav1dde force-pushed the dav1d/server-connection-timeout branch 2 times, most recently from f92738f to de4b5b1 Compare November 13, 2024 21:13
@Dav1dde Dav1dde force-pushed the dav1d/server-connection-timeout branch from de4b5b1 to eca7427 Compare November 13, 2024 21:13
CHANGELOG.md Outdated Show resolved Hide resolved
relay-server/src/services/server/acceptor.rs Outdated Show resolved Hide resolved
Comment on lines +31 to +32
timer: None,
is_idle: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work with a single variable deadline: Instant, i.e. let Poll::Ready reset the deadline to a point in the future and Poll::Pending check it?

Is the usage of a timer + boolean an optimization to reduce calls to Instant::now()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does reduce the amount of instants created and I also wanted the timer to only start when you poll a Pending. I initially had a Option<Sleep> implementation, but that re-created the Sleep a bunch which is especially expensive with the Box::pin. In the end that's the approach I liked most.

@Dav1dde Dav1dde requested a review from jjbayer November 14, 2024 08:36
@Dav1dde Dav1dde self-assigned this Nov 14, 2024
/// the [`IdleTimeout`] will abort the operation and return [`std::io::ErrorKind::TimedOut`].
pub struct IdleTimeout<T> {
inner: T,
timeout: Option<Duration>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this would make typing annoying, but it would be cool if timeout was non-optional here. So if there's no timeout set in the config, don't use IdleTimeout.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works if we instead use Box<dyn AsyncRead + AsyncWrite + Unpin> as the concrete type returned from the Acceptor or introduce an Either<IdleTimeout<TcpStream>, TcpStream> type. I can do that in a follow-up.

@Dav1dde Dav1dde merged commit 013eca3 into master Nov 14, 2024
23 checks passed
@Dav1dde Dav1dde deleted the dav1d/server-connection-timeout branch November 14, 2024 10:01
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

Successfully merging this pull request may close these issues.

2 participants