Please elaborate the intended use pattern of new soundness APIs

[complexspaces]

Hello,

In https://github.com/time-rs/time/commit/5c4716d6ff1fcbf4a2c32ef84090fe7d204a2b64, the old opt-in for potentially unsound behavior on UNIX platforms was removed in favor of a runtime API instead. However, this API does not seem to provide downstream users with the same flexibility as before.

Multiple comments seem to indicate that it should be possible to have it work like before, but the new "abort the program if the user would have invoked undefined behavior" check is, AFAICT, overly zealous because it does not consider the safety requirements of set_soundness. Instead, it just assumes that a multi-threaded environment on UNIX systems has continual environment mutation occurring instead of relying on the user to uphold the conditions.

The result of this is that, even in an application where multiple threads are not problematic, it has become not possible to obtain the local offset. If you have the bandwidth available, would you be able to elaborate more on the new API and let me know if I'm simply holding it wrong?

[jhpratt]

I'll need to take a second look at this. Unfortunately it's a really difficult problem to solve, and it's certainly possible I went a bit too far in enforcing things.

[mathieulj]

Just my 10¢ but I think the api might be more usable if we would have comparable conditions in debug as we do in release. i.e. if instead of

    // In release mode, let the user invoke undefined behavior if they so choose.
     #[cfg(not(debug_assertions))]
     if local_offset::get_soundness() == Soundness::Sound
         && num_threads::is_single_threaded() != Some(true)
     {
         return None;
     }
     // In debug mode, abort the program if the user would have invoked undefined behavior.
     #[cfg(debug_assertions)]
     if num_threads::is_single_threaded() != Some(true) {
         if local_offset::get_soundness() == Soundness::Unsound {
             eprintln!(
                 "WARNING: You are attempting to obtain the local UTC offset in a multi-threaded \
                  context. On Unix-like systems, this is undefined behavior. Either you or a \
                  dependency explicitly opted into unsound behavior. See the safety documentation \
                  for `time::local_offset::set_soundness` for further details."
             );
             std::process::abort();
         }
         return None;
     }

We do

     if local_offset::get_soundness() == Soundness::Sound
         && num_threads::is_single_threaded() != Some(true)
     {
         // In debug mode, abort the program if the user would have invoked undefined behaviour.
         #[cfg(debug_assertions)]
         std::process::abort();

         // In release mode, return None rather than invoking undefined behaviour.
         #[cfg(not(debug_assertions))]
         return None;
     }

Since set_soundness is unsafe, we are essentially allowing the client application to assert that they are vouching for the soundness of their runtime in this condition. Sadly the unsafe may not deter all libraries from setting this themselves but it's something that would stand out during a dependency review.

[jhpratt]

If I took that approach, the only thing enabling the flag would do is to add an abort in debug mode. The entire point of the soundness API is to avoid the multi-threaded check, which is why it is currently only performed in debug mode.

I have not stated this yet, but my intent is to adopt the suggestion from @complexspaces and remove the abort (and error message).

[jhpratt]

Done on main.

[thomcc]

             eprintln!(
                 "WARNING: You are attempting to obtain the local UTC offset in a multi-threaded \
                  context. On Unix-like systems, this is undefined behavior. Either you or a \
                  dependency explicitly opted into unsound behavior. See the safety documentation \
                  for `time::local_offset::set_soundness` for further details."
             );

Hmm, this isn't quite accurate. It's not a sound API if there are multiple threads, but not all uses of the API would lead to undefined behavior, only if it's actually used from multiple threads, no?

That said, I'm just being pedantic, because set_soundness exists this doesn't really matter.

[jhpratt]

That is correct, and is why the behavior has already changed on main.