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

Fix time issues #1605

Merged
merged 7 commits into from
Dec 29, 2023
Merged

Fix time issues #1605

merged 7 commits into from
Dec 29, 2023

Conversation

huitema
Copy link
Collaborator

@huitema huitema commented Dec 28, 2023

Issue #1601 points out that using gettimeofday() for managing timers can cause errors if the local admin resets that time. It is preferable on Linux to use clock_gettime(CLOCK_MONOTONIC) for that purpose. This requires separating the TLS related usage of time, which has to track "wall time", from the timer-related usage prevalent in QUIC protocol handling.

The solution here is:

  • make sure that ticket and token related calls use picoquic_get_tls_time(quic), and rewrite the token and ticket API so as to not take a time parameter;
  • incorporate the patch proposed by @fbussery and use CLOCK_MONOTONIC for other protocol actions.

@huitema
Copy link
Collaborator Author

huitema commented Dec 28, 2023

Last point: clock_gettime may in some cases return an error, but merely reading the clock should only cause an issue if the calling arguments are malformed. I would rather ignore the return code than create an error branch that is never exercised.


/* Load the empty file again */
if (ret == 0) {
ret = picoquic_load_tickets(&p_first_ticket_empty, retrieve_time, test_ticket_file_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

p_first_ticket_empty still defined in line 111. Not needed any more

@huitema huitema merged commit 510e031 into master Dec 29, 2023
11 checks passed
@huitema huitema deleted the pathch-tie-issues branch December 29, 2023 06:26
@@ -5116,7 +5116,7 @@ int virtual_time_test()
{
/* Check that the simulated time follows the simulation */
for (int i = 0; ret == 0 && i < 5; i++) {
simulated_time += 12345000;
simulated_time += 12345678000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a doubt. In my mind, simulated_time was of same unity as picoquic_get_quic_time which did not change

@huitema
Copy link
Collaborator Author

huitema commented Dec 29, 2023

I also have a doubt. The "gettimeofday" approach has the nice property of inheriting clock synchronization, NTP, etc. This is not strictly required for the transport, but it does make debugging easier, e.g., when comparing time stamps at the client and the server. I wonder whether the use of monotonic clock should be controlled by a compile option.

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