Conversation
Signed-off-by: aeryz <abdullaheryz@protonmail.com>
There was a problem hiding this comment.
I can do a proper review once this is no longer a draft. @Arshia001 @theduke what do you think of this change?
Please make sure this does not break existing use of proc_wasix_interval. When you add the tests for proc_raise_interval and proc_raise_interval2 add them in lib/wasix/tests/wasm_tests and not in tests/wasix.
Also the proc_raise_interval2 implementation should live in its own file in syscalls/wasix.
Usually we make the old version just forward to the new implementation of the syscall but this might be hard here given the new signature reads the values from memory. Maybe create a shared helper or adjust the signature?
I agree that we should add documentation on the limitations of our signal handling implementation,. For now a comment to the syscall implementation is probably good enough.
theduke
left a comment
theduke
left a comment
There was a problem hiding this comment.
Thanks!
The direction makes sense.
Needs some modifications.
| } else { | ||
| zero | ||
| }, | ||
| value: __wasi_timeval_t { |
There was a problem hiding this comment.
If we are sticking to Linux semantics of setitimer here, which I reckon we should, then the old value is supposed to be the time until the next trigger time, not the originally configured time.
There was a problem hiding this comment.
Oh good catch! My understanding was when a timer is hit, the value becomes interval and interval stays as is. It's actually true but I missed the part where value always gives the time until the next trigger time.
Relevant info: posix settimer, setitimer.
I'll return the value - now - last_signal instead.
| ) -> Result<Errno, WasiError> { | ||
| println!("called regular raise"); | ||
| let env = ctx.data(); | ||
| let interval = match interval { |
There was a problem hiding this comment.
As @zebreus said, an we implement a helper function that uses Rust types, and make both old and new implementation use the helper, to avoid duplication?
There was a problem hiding this comment.
Yeah yeah, I wanted to open a PR to have a minimal PoC on the new syscall. Since we are good with the API now, I will do the refactors and add the tests to complete the PR.
There was a problem hiding this comment.
Giving it some more thought, I think the functions are currently already implemented in a way that the main logic is shared. Note that the shared logic is:
- timer is set with the given signal
- previous timer is removed when the timer.value is
0
Both of these are already implemented byWasiProcess::signal_interval. The functions differ in: proc_raise_intervalworks with millisecond timestamps, butproc_raise_interval2works withitimervalproc_raise_interval2makes use of the old timer.
These are already implemented seperately and the only duplicate part isWasiEnv::do_pending_operationswhich I would say it's good to keep it explicit per syscall unless the legacy implementation is directly forwarded to the new implementation.
And in terms of the use of proper Rust types. I kept the fields of __wasi_timeval_t private and make it convertible to Duration. So if anyone wants to do any meaningful thing with __wasi_timeval_t, they would have to convert it to Duration first. And our internal Rust types and the signal interval related functions all use Durations. Please let me know if you see a possible footgun.
| } else { | ||
| Some(Duration::from_secs(tv.sec) + Duration::from_micros(tv.usec)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should add a test/tests in the tests/wasix test suite.
The pattern of existing tests should be easy to emulate.
Note: you'll need wasixcc ( https://github.com/wasix-org/wasixcc )
There was a problem hiding this comment.
Tests are added under libc_tests for alarm and setitimer.
| } | ||
|
|
||
| fn timeval_to_duration(tv: __wasi_timeval_t) -> Option<Duration> { | ||
| if tv.sec == 0 && tv.usec == 0 { |
There was a problem hiding this comment.
I believe this can overflow the range of Duration.
Should probably return an error here, so the caller gets back an Errno::INVAL.
There was a problem hiding this comment.
Yeah better to use checked_add here
3 participants
Upon investigating #5907, I realized that proc_raise_interval is not sufficient for a complete setitimer implementation. The details are explained in depth here.
Under the issue, @zebreus suggested that we introduce a new syscall called
proc_raise_interval2which closely followssetitimer.This PR is an attempt to add it. Since I'm new here and don't know the process of introducing a new API, I wanted to create the draft PR and show how would it look like. In addition to this PR, here are the changes done to other repos:
Additionally,
proc_raise_intervalsyscall was previously missing the support for repetition which is now supported for bothproc_raise_intervalandproc_raise_interval2.Fixes: #6284
Also note that, due to the sync nature of how the signal handling works, the runtime only checks for the intervals during syscalls. This means on CPU-heavy applications, the signal timer will not work properly (it will never execute before the interval but might take longer). We can note this as a known limitation for now and in the future work on async signal handling.
If we agree on adding this syscall, I will also add some e2e tests.