Conversation
|
credit goes to @i-am-logger for the original PR here: #718 the main difference between this PR and his is that this one does not touch PCAP or QMDL at all. I think it's too confusing to mess with it. The metadata of a recording is adjusted though (at recording time) |
d7591ac to
54760bd
Compare
wgreenberg
left a comment
There was a problem hiding this comment.
one comment and a few suggestions to clarify the UI text
| use chrono::{DateTime, Local, TimeDelta}; | ||
| use std::sync::RwLock; | ||
|
|
||
| static CLOCK_OFFSET: RwLock<TimeDelta> = RwLock::new(TimeDelta::zero()); |
There was a problem hiding this comment.
for this, do you think we ought to use the tokio version of RwLock? that way we avoid synchronous blocking in our async code
There was a problem hiding this comment.
Then set_offset etc have to become async, but these functions only do sync operations.
I think we should consider using async RwLock if we do async work while holding the lock. But for setting a global, that's barely any work at all.
I was considering making CLOCK_OFFSET an atomic int of some sort but it just makes the code worse.
There was a problem hiding this comment.
i agree that the get/set functions do a minimal amount of work, but since get_adjusted_now() is used somewhat often (and may be used more in the future), i worry about cases where we have a large number of reads active at a given time, which would cause set_offset() to synchronously block (and vice versa). the knock-on effects of little sync blocks in a single-threaded async program are also hard to predict and debug
IMO this makes the slight annoyance of making get/set_offset async is worth it to avoid synchronous blocks
There was a problem hiding this comment.
I think this can be correct in theory but we just don't have that much concurrency in rayhunter overall where one lock would wait forever for another lock to release. we may have a lot of sequential calls, which are fine.
there's another more subtle point though: we use the single-threaded tokio runtime (set in main.rs). since the critical section of the lock is sync, it blocks the entire runtime. so other tasks cannot execute, so they cannot cause any contention at all. this is independent of which lock type we chose.
tokio's own docs recommend mutex in this case: https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use
This makes the async mutex more expensive than the blocking mutex, so the blocking mutex should be preferred in the cases where it can be used.
I think internally the async mutex also uses a sync mutex for its own bookkeeping, so its overhead is strictly higher
When there is a significant difference between the user's browser's time and the system time, a button appears in the web UI to fix the system time. This time will then be used to correct both data inside of PCAPs and any metadata. We don't actually set the system time to this value. Instead, rayhunter adjusts any timestamps it handles by an offset. That offset defaults to zero, and the user adjusts it by hitting the button in the web UI. The main reason for this is device portability. I haven't investigated whether it would actually be easy to set the real system time. It's possible that it works the same way across all devices.
Co-authored-by: Will Greenberg <willg@eff.org>
I haven't looked at the code, but without applying this branch my pcaps start at |
|
@kmille thanks for calling that out, I really thought I got rid of that code. But this shows another issue, if we want to correct modem timestamps we have to store a separate offset. Since a bad clock starts at 1980 for the modem, and at 1970 for the rest of the system. |
|
Easiest solution is probably adding an |
When there is a significant difference between the user's browser's time
and the system time, a button appears in the web UI to fix the system
time. This time will then be used to correct both data inside of PCAPs
and any metadata.
We don't actually set the system time to this value. Instead, rayhunter
adjusts any timestamps it handles by an offset. That offset defaults to
zero, and the user adjusts it by hitting the button in the web UI. The
main reason for this is device portability.
I haven't investigated whether it would actually be easy to set the real
system time. It's possible that it works the same way across all
devices.
Fix #121