-
Notifications
You must be signed in to change notification settings - Fork 169
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
Fix time issues #1605
Conversation
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); |
There was a problem hiding this comment.
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
@@ -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; |
There was a problem hiding this comment.
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
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. |
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 useclock_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:
picoquic_get_tls_time(quic)
, and rewrite the token and ticket API so as to not take a time parameter;CLOCK_MONOTONIC
for other protocol actions.