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

Align the server and client idle timeouts in Ember #7329

Merged

Conversation

rlavolee
Copy link
Contributor

Fixes: #7327

val idleConnectionTime: FiniteDuration = org.http4s.client.defaults.RequestTimeout
val timeout: Duration = org.http4s.client.defaults.RequestTimeout
val idleConnectionTime: FiniteDuration = org.http4s.ember.core.Defaults.idleTimeout
val timeout: Duration = org.http4s.ember.core.Defaults.idleTimeout
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe changing the default timeout is not desired. I was thinking that 45s is quite a lot but 60s 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad 😬 . I reverted the timeout from previous value.

import scala.concurrent.duration.FiniteDuration

private[ember] object Defaults {
val idleTimeout: FiniteDuration = 60.seconds
Copy link
Member

Choose a reason for hiding this comment

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

We have different notations for such vals over the codebase, but maybe PascalCase better fits. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting. I saw camelCase and PascalCase vals in this project but how do you choose between eatch other ?

@danicheg
Copy link
Member

I agree with having a single source of truth for general client/server defaults. I doubt a little about putting it into ember-core, but we don't have the ember-common, and it's unlikely worth the hassle. So it seems to be an applicable trade-off. Eager to hear opinions from the team.

Copy link
Member

@valencik valencik left a comment

Choose a reason for hiding this comment

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

Thanks @rlavolee for your efforts here.

The technical approach seems fine.
Because this is a change of behaviour, we need to remember to call it our clearly in our next release notes.

(edit: just wanted to link the brief discussion about whether this change can go in a patch release: #7327 (comment))

@danicheg
Copy link
Member

danicheg commented Dec 2, 2023

Thanks again @rlavolee!

@danicheg danicheg merged commit 3ba79a7 into http4s:series/0.23 Dec 2, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align the server and client idle timeouts in Ember
3 participants