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 #7327

Closed
danicheg opened this issue Nov 21, 2023 · 7 comments · Fixed by #7329
Closed

Align the server and client idle timeouts in Ember #7327

danicheg opened this issue Nov 21, 2023 · 7 comments · Fixed by #7329
Assignees
Labels
enhancement Feature requests and improvements good first issue A tractable issue for those looking to make an initial contribution module:ember-client

Comments

@danicheg
Copy link
Member

As things stand in v0.23.24, the server has a default idle timeout set to 60s against 45s for the client. From my understanding, it's better to have them equal by default.

@danicheg danicheg added enhancement Feature requests and improvements good first issue A tractable issue for those looking to make an initial contribution module:ember-client labels Nov 21, 2023
@rlavolee
Copy link
Contributor

I can take a look. Should the default value be set to 60s or 45s ?
Can I add the idle timeout value in core/shared to share it between client and server ?

@danicheg
Copy link
Member Author

Great! Thank you @rlavolee.

Should the default value be set to 60s or 45s ?

We haven't discussed this with a team, but from my understanding, 60s is a fine option.

Can I add the idle timeout value in core/shared to share it between client and server ?

That's a good question. On the one hand, it's the right approach to place these common settings into some shared module (I'd argue that ember-core is a better choice). On another hand, these settings are barely applicable to the core module in the sense it's designed. But we don't have an ember-common module, and I'm neither confident whether it's worth the hassle. Perhaps putting these as package private inhabitants into ember-core is a suitable trade-off. But I'd like to get more opinions from the team on that. If you open the PR, it'd be a great place to discuss this in detail.

@rlavolee
Copy link
Contributor

I was thinking about core because ember-client and ember-server depends on client and server which depends on core. However if it's specific to ember then ember-core is great. I'll open the PR 👍 . Thanks

@valencik
Copy link
Member

Changing a default value could be a behaviour change for some systems, is this something we're fine with in a patch change?
I don't really know how folks view default values in relation to the stable API surface.
(this isn't me objecting, just wondering if we have precedent here)

Otherwise, I think having the timeouts be the same is reasonable.

@danicheg
Copy link
Member Author

As we previously agreed (don't have a reference at hand tho), there will be no 0.24-series. So we ship everything as patch releases.

@armanbilge
Copy link
Member

there will be no 0.24-series. So we ship everything as patch releases.

Well, there is nuance here :) we don't ship everything as patch releases. For example, we don't ship binary-breaking changes in patch releases.

The question Andrew raises is if this change is significant or breaking enough that it is not appropriate for a patch release. I think this is a reasonable question.

On balance, I think it is okay to ship this change in a patch:

  1. it only affects Ember client.

  2. it is increasing the timeout. So it will not create new timeouts for users that are currently calling slow services. However, this change will negatively impact load-shedding when the connection has truly failed.

  3. 45 seconds to 60 seconds still feels relatively small. We're not increasing it to 450 seconds or something 😂

@danicheg
Copy link
Member Author

danicheg commented Nov 28, 2023

we don't ship everything as patch releases. For example, we don't ship binary-breaking changes in patch releases.

What a pedantry! 😄 It's out of the reality that we ship something not binary-compatible in patch releases. (Thankfully, Mima would break hands otherwise)
It's also true that it's barely that someone could treat changing the value of a constant in the library as a non-binary-compatible change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements good first issue A tractable issue for those looking to make an initial contribution module:ember-client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants