Skip to content

Consider not using the wall clock time for timeouts in Kestrel #13628

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

Closed
Tragetaschen opened this issue Sep 2, 2019 · 18 comments · Fixed by #48081
Closed

Consider not using the wall clock time for timeouts in Kestrel #13628

Tragetaschen opened this issue Sep 2, 2019 · 18 comments · Fixed by #48081
Assignees
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel help wanted Up for grabs. We would accept a PR to help resolve this issue severity-major This label is used by an internal tool
Milestone

Comments

@Tragetaschen
Copy link
Contributor

Is your feature request related to a problem? Please describe.

I have an embedded instrument with an RTC that's powered by a small gold cap. However, that battery is limited and so the instrument might forget the current date / time. If that happens, the system boots with a default date/time depending on when the Linux image was built.

There's a user interface that's rendered on an internal web browser connected to the localhost web server via SignalR.

Third, there's a way to set the date/time when lost, which directly sets the wall clock on the instrument (the time difference is typically O(months)).

When the user sets the date/time, the SignalR connection runs into a timeout because timeouts are based on wall clock time and my thought-to-be stable localhost connection breaks. I have included a retry logic just for that case.

Describe the solution you'd like

Using wall clock for timeouts always runs into scenarios such as the one described above. In my opinion it would be better to base those decisions off of a monotonic and continuous time source (Stopwatch?).

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Sep 3, 2019
@halter73
Copy link
Member

halter73 commented Sep 3, 2019

Do you know if Stopwatch is actually guaranteed to be monotonic and continuous?

@halter73 halter73 added this to the Backlog milestone Sep 3, 2019
@Tragetaschen
Copy link
Contributor Author

Tragetaschen commented Sep 4, 2019

My read on the documentation is that Stopwatch uses a monotonic counter as a backing store and looking through the code, that claim is backed:

  • Windows uses QueryPerformanceCounter and debug-asserts that it works (available since XP)
  • On Linux it uses a CLOCK_MONOTONIC, again with an assert.
  • On Mac, there is mach_absolute_time

I think the only time when it was not guaranteed is the pre-XP operating systems.

@halter73
Copy link
Member

halter73 commented Sep 4, 2019

If we change this is SignalR, we should probably consider doing the same for Kestrel timeouts. While it's probably not as likely to run on client devices with unreliable clocks like SignalR, it seems like the right thing to do.

@BrennanConroy
Copy link
Member

This specific case looks like the server was running on one of those devices so Kestrel would definitely have similar issues.

@samsosa
Copy link
Contributor

samsosa commented Sep 6, 2019

Maybe Environment.TickCount or Environment.TickCount64 are good (and fast) enough?

@Tragetaschen
Copy link
Contributor Author

Tragetaschen commented Sep 7, 2019

This is probably worth synchronizing with: dotnet/runtime#15207

@Tragetaschen
Copy link
Contributor Author

Related to dotnet/corefx#35401

@BrennanConroy
Copy link
Member

Related to dotnet/corefx#35401

Related to that dotnet/corefx@06f25ea where they switched to the new Environment.TickCount64 API.

Seems like a good possible mechanism to switch to.

@BrennanConroy BrennanConroy added affected-very-few This issue impacts very few customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-major This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@ghost
Copy link

ghost commented Apr 6, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl
Copy link
Member

This feels like something we should do.

@BrennanConroy
Copy link
Member

Agreed, SignalR and Kestrel should switch to using Environment.TickCount64 for timeouts.

@halter73
Copy link
Member

The analogue for this in the SignalR JS/TS client is performance.now().

@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Apr 26, 2021
@BrennanConroy BrennanConroy removed the area-signalr Includes: SignalR clients and servers label Jul 29, 2021
@adityamandaleeka adityamandaleeka changed the title Consider not using the wall clock time for timeouts Consider not using the wall clock time for timeouts in Kestrel Aug 11, 2021
@adityamandaleeka
Copy link
Member

Updated the title since the SignalR side of this is done in #34382

@ghost
Copy link

ghost commented Aug 11, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@BrennanConroy
Copy link
Member

😢 I should push a branch for the Kestrel stuff I tried

@adityamandaleeka
Copy link
Member

Sure, and I'm not ruling this out for 7. Just moved it out of NSP since it's too late for 6.

@halter73
Copy link
Member

😢 I should push a branch for the Kestrel stuff I tried

This isn't an API change. I see no reason we couldn't take this for 6 if it's ready.

@adityamandaleeka
Copy link
Member

Agreed. It's not ready though, and it sounds like it's trickier than originally imagined. @BrennanConroy if you are still interested in getting it done for 6, don't let the milestone change stop you :).

@Tratcher Tratcher self-assigned this Apr 17, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel help wanted Up for grabs. We would accept a PR to help resolve this issue severity-major This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants