Change thread local variables to only accept async-signal-safe types.

[mahkoh]

• Add the Interrupt trait for thread-local objects that can be used
  from signal handlers.
• Relax the requirements for #[thread_local] statics from Sync to
  Interrupt.

[eefriedman]

Just emitting the locking instructions isn't sufficient; we would actually have to use atomic increment/decrement. (An async signal handler could increment the borrow count of a RefCell by stashing the return value of borrow() into a global.)

[eefriedman]

Hmm... correction; it's okay for thread_local!() because it's impossible to stash away a reference to a thread-local variable. (You end up with potentially wacky behavior if you leak a Ref in a signal handler, but not unsafe behavior.)

It's not obvious that property will still hold for #[thread_local], though.

Even if we don't need an atomic increment, I'm not sure it's appropriate to completely ignore the performance cost of the optimization barrier.

[mahkoh]

It's not obvious that property will still hold for #[thread_local], though.

Without a 'thread lifetime #[thread_local] variables are already unsafe because they pretend to be 'static.

Even if we don't need an atomic increment, I'm not sure it's appropriate to completely ignore the performance cost of the optimization barrier.

It's certainly easy to write another type that replaces RefCell if this is true. In this case the fix would be to replace the name RefCell by the name of its signal-safe replacement.

[mahkoh]

I believe the problem with forgetting borrows or putting them into globals can be fixed without atomic operations. RefCell currently looks like this:

pub struct RefCell<T: ?Sized> {
    borrow: Cell<BorrowFlag>,
    value: UnsafeCell<T>,
}

impl<T: ?Sized> RefCell<T> {
    pub fn borrow(&self) -> Ref<T> {
        match BorrowRef::new(&self.borrow) {
            Some(b) => Ref {
                _value: unsafe { &*self.value.get() },
                _borrow: b,
            },
            None => panic!("RefCell<T> already mutably borrowed"),
        }
    }
}

And it can be made safe like this:

pub struct RefCell<T: ?Sized> {
    locked: Cell<bool>,
    borrow: Cell<BorrowFlag>,
    value: UnsafeCell<T>,
}

impl<T> RefCell<T> {
    pub fn borrow(&self) -> Ref<T> {
        if self.locked.get() == false {
            self.locked.set(true);
            barrier();
            if let Some(b) = BorrowRef::new(&self.borrow) {
                barrier();
                self.locked.set(false);
                return Ref {
                    _value: unsafe { &*self.value.get() },
                    _borrow: b,
                };
            }
        }
        panic!("RefCell<T> already mutably borrowed");
    }
}

But since this is a more complicated fix than adding a single barrier, it might be better to replace RefCell by another type. Note that the code above can be written better with the correct abstractions:

pub struct RefCell<T: ?Sized> {
    borrow: SingleThreadMutex<BorrowFlag>,
    value: UnsafeCell<T>,
}

impl<T> RefCell<T> {
    pub fn borrow(&self) -> Ref<T> {
        match BorrowRef::new(self.borrow.lock()) {
            Some(b) => Ref {
                _value: unsafe { &*self.value.get() },
                _borrow: b,
            },
            None => panic!("RefCell<T> already mutably borrowed"),
        }
    }
}

But a SingleThreadMutex that is general purpose might be slightly less efficient than the previous version.

[mahkoh]

An async signal handler could increment the borrow count of a RefCell by stashing the return value of borrow() into a global.

It actually can't because globals cannot contain types with destructors. This means that the more complicated fix above is unnecessary and the original proposal in the RFC should work.

[eefriedman]

I wrote a bit async signal handlers in the standard library here: rust-lang/rust#30003 (comment) . Basically, I don't see safe async signal handlers ever happening in the Rust standard library in the way you're proposing.

One more thing worth pointing out: if you're going to throw out POSIX, there isn't any reason to have async signal handlers run on an existing thread.

[mahkoh]

Basically, I don't see safe async signal handlers ever happening in the Rust standard library in the way you're proposing.

This RFC does not propose adding any kind of async signal handling to the standard library (see also the last alternative.) It proposes adding the right tools to the language to make all rust code async signal safe by default as long as all the ffi code it calls is async signal safe. The language already does exactly the same with thread-safety.

Is there more code out there that is async signal unsafe than there is code out there that is thread unsafe? Very likely.

Is it possible to use the language in a way that only interacts with async signal safe ffi code? Definitely.

Is it possible that, in the future, more ffi code will be written that is async signal safe? Certainly if this RFC is accepted.

If K&R had made C async signal safe by default 40 years ago, then we would not be having this discussion.

One more thing worth pointing out: if you're going to throw out POSIX, there isn't any reason to have async signal handlers run on an existing thread.

Sounds like an incorrect generalization.

• Async signals can be used to interrupt a thread that is doing uninterrupted work in a loop. Synchronous signal handling would require additional atomic operations (at best) in every iteration.
• Certain signals cannot be handled in a synchronous way.
• Sometimes there is only one thread.
• Signals can be used to interrupt arbitrary user code/threads to update thread-local state.
• Threading libraries reserve various signals (NPTL: 2; LinuxThreads: 3) for thread synchronization.

[wthrowe]

The code proposed here doesn't compile. As far as rustc is concerned, the "Sync" impl conflicts with all the negative impls.

Even if the language were modified sufficiently so that these impls did not conflict, under the current implementation of OIBITS neither the default impl nor any of the negative impls ever applies. Aside from the compilation errors, I believe this is equivalent to

unsafe trait Interrupt {}
unsafe impl<T: Sync> Interrupt for T {}

I continue to oppose proposals that add OIBITS until we at least have some sort of consensus on whether the current implementation is a bug or not.

A comment on the RFC text itself: I believe this is proposing that RefCell should be Interrupt, but this doesn't appear to actually be explicitly stated anywhere.

[mahkoh]

The code proposed here doesn't compile. As far as rustc is concerned, the "Sync" impl conflicts with all the negative impls.

You have to put it in libcore. Otherwise it won't compile. I've already implemented it in lrs and it compiles just fine there. But maybe this is a bug and it should never compile?

A comment on the RFC text itself: I believe this is proposing that RefCell should be Interrupt, but this doesn't appear to actually be explicitly stated anywhere.

Give the performance concerns @eefriedman has, it is not clear whether RefCell should be fixed or if the library should be supplemented by another type that is signal safe. It is possible to implement a SingleThreadMutex that does not have the move-into-global problem mentioned by @eefriedman above and that compiles down to a single branch:

f:
    movb    (addr), %al
    movb    $1, (addr)
    orb     1(addr), %al
    jne     .panic
    movb    $1, 1(addr)

    # your code here

    movb    $0, 1(addr)
    movb    $0, (addr)
    retq
.panic:
    # panic

[wthrowe]

You have to put it in libcore. Otherwise it won't compile. I've already implemented it in lrs and it compiles just fine there. But maybe this is a bug and it should never compile?

Ah, no, you're right. I forgot part of the coherence rules.

I still believe the negative impls don't actually do anything, though.

[mahkoh]

I still believe the negative impls don't actually do anything, though.

The simple two-liner you posted above might actually be the better implementation since Sync: Interrupt.

unsafe trait Interrupt {}
unsafe impl<T: Sync> Interrupt for T {}

And then one just has to add additional positive implementations for things that are not quite Sync but Interrupt.

Edit: Nope, I was wrong. Not having a .. impl breaks it.

[mahkoh]

Since the previous version was too complicated I've reduced the RFC to its core. The old version can still be accessed via the commit history.

The new version only deals with the language aspects of async-signal-safety: The types that can be the type of a #[thread_local] variable are exactly those which implement Interrupt. It no longer dictates how the trait is used, which types implement it, etc. In particular:

• If it gets a default implementation.
• If Interrupt: Sync is defined so that only Sync types can be put into #[thread_local] variables. In this case, literally nothing changes for current users of #[thread_local] and the requirements can be relaxed in the future. (Since everything is unstable.)
• If the thread_local! macro makes any use of the trait.
• If any types are changed to become Interrupt.

This allows people who are more familiar with the constraints of different libraries to decide how the distinction between Interrupt and Sync is used. If it is like @eefriedman says and the standard library will never be signal safe, then

unsafe impl Interrupt for .. { }

without any negative implementations might be the way to go. lrs, which does not depend on any legacy C code, will be able to make better use of it.

The RFC now no longer proposes any breaking changes.

[mahkoh]

@alexcrichton I believe this should be T-lang and not T-libs.

[nikomatsakis]

Overall, I am interested in addressing signal handling in a more first-class fashion. It's come up also in the context of embedded devices. This also seems quite related to the idea of cooperative multitasking on a single thread, which has looser requirements than true multithreading, and is also quite useful in embedded devices and so forth. This is obviously related to the general topic of this RFC, though it's not clear that the actual trait proposed here would be especially helpful for anything other than #[thread_local] variables. Do you have other uses in mind, @mahkoh ?

[mahkoh]

I have not but I think the trait (as formulated in the RFC) can also handle voluntary context switches. What it cannot handle are general involuntary context switches, e.g. userspace scheduling via signals.

[alexcrichton]

🔔 This RFC is now entering its week-long final comment period 🔔

[CodesInChaos]

Could you explain why thread-safety (Sync) implies signal-handler-safety (Interupt)?

[kennytm]

What types are Interrupt and not Sync?

[alexcrichton]

The libs team got a chance to discuss this RFC yesterday and the conclusion was that we're going to close this for now. As @nikomatsakis mentioned it definitely seems worthwhile to ensure that we support signals and such nicely, but at this time we think we may want to flesh out the story there a little more thoroughly as well.

Thanks regardless though for the RFC @mahkoh!