Skip to content
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

When Multiple Signal Handlers Register, They Step on One another #170

Open
sargun opened this issue Apr 5, 2018 · 10 comments
Open

When Multiple Signal Handlers Register, They Step on One another #170

sargun opened this issue Apr 5, 2018 · 10 comments

Comments

@sargun
Copy link

sargun commented Apr 5, 2018

If you try to register signal handlers multiple times for the same signal, the last one wins. This can lead to undefined behaviour if libraries use signal handlers.

@mfelsche
Copy link
Contributor

mfelsche commented Apr 5, 2018

Thanks for reporting.
I quickly wanted to check on this and it is true, that only 1 signal handler is called for 1 distinct signal.

Here is a minimal example to show this behaviour:

use "signals"

actor Main
  new create(env: Env) =>
    let handler1 = SignalHandler(
      object iso is SignalNotify
        fun ref apply(count: U32): Bool =>
          env.out.print("1")
          true
      end,
      Sig.usr1()
    )
    let handler2 = SignalHandler(
      object iso is SignalNotify
        fun ref apply(count: U32): Bool =>
          env.out.print("2")
          true
      end,
      Sig.usr1()
    )
    raise()

  be raise() =>
    SignalRaise(Sig.usr1())

The behaviour on how multiple signal handlers for the same signal behave is not documented in the signals sources or docs. If it were, i would say this should be turned into an RFC, allowing multiple signal handlers for the same signal (with undefined order). But as there is no documentation, this could be handled as a principle of least surprise enhancement without RFC. But maybe others have a different opinion on this.

@Praetonus
Copy link
Member

After taking a look at the signal implementation (asio/epoll.c and asio/kqueue.c), the behaviour you describe seems to correspond to the OSX implementation. This implementation is, as you point out, not thread safe, and I seem to recall another issue opened about that problem. I can't find it though.

The Linux implementation also has problems, and isn't fully thread safe (though it's better than the OSX implementation). On that platform, the first signal handler registered wins, and cannot be replaced. Every other signal handler created for the same signal will silently do nothing.

In addition, both implementations have a problem with signals like SIGFPE, SIGILL and SIGSEGV (and SIGABRT when raised by abort()), as these signals are required to terminate the program after the signal handler returns. Since our signal handler delegates things to epoll/kqueue, this means that user handlers for these signals won't be called.

The Pony interface in the standard library is problematic as well, as it isn't capability secure. Any function can raise a signal or register a signal handler without first getting the capability to do so.

All in all, I think signals are pretty much broken right now, both from a design and from an implementation standpoint.

Aside from fixing the thread safety issues and the capability security of the interface, I think the questions here are:

  • As @mfelsche said, what should the behaviour be when trying to register multiple signal handlers? If we only allow one at a time, the replacement needs to be capability secure (i.e. a function should need something derived from AmbientAuth in order to replace a signal handler).
  • What should signal handlers for fatal signals look like? Should we even allow users to handle them?

@SeanTAllen
Copy link
Member

SeanTAllen commented Apr 5, 2018 via email

@Praetonus
Copy link
Member

@SeanTAllen It could be handled by the runtime. For example by storing a list of ASIO events for each signal, instead of just one.

@SeanTAllen
Copy link
Member

I wouldnt consider what it does now broken, it it what I as someone unfortunate enough to use signals a lot in the past would expect. That said, if we could normalize that, it would be great.

@Praetonus
Copy link
Member

The main argument I have for treating this as a bug is that the Linux and OSX implementations currently do opposite things.

@SeanTAllen
Copy link
Member

well, Linux and OSX do different things. So I wouldnt call it a bug. But I think that if we can address, that would be a good thing.

@sargun
Copy link
Author

sargun commented Apr 5, 2018

@Praetonus I'm somewhat ignorant, since I've only been reading the code for a couple hours, how is the OS X kqueue implementation thread unsafe?

It looks like ponyint_asio_backend_dispatch is executed serially. Is it that pony_asio_event_subscribe has no locking with it and that can create skew?

@mfelsche
Copy link
Contributor

We discussed this during sync and decided to move the discussion about how the signal interface should look like to an RFC ticket.

@Praetonus
Copy link
Member

@sargun Sorry, I forgot to reply to you here. I was incorrect, the OSX implementation is indeed thread safe. The other problems are still relevant, though.

@SeanTAllen SeanTAllen transferred this issue from ponylang/ponyc May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants