Add mechanism to register (noisy) signal handler #2631

sargun · 2018-04-05T07:16:31Z

This allows programs that are waiting on a signal to proceed to
work. Sometimes, there are programs that are waiting for an alarm,
or similar prior to performing I/O.

This was impossible, because signal handlers were never registered
as noisy actors in asio. This allows for an optional named parameter
to be passed with that context.

A simple example of a program that was not supposed to terminate until it received a signal:

use "signals"
use "bureaucracy"

actor Main
  new create(env: Env) =>
    // Create a TERM handler
    let custodian = Custodian
    let signal = SignalHandler(TermHandler(env, custodian), Sig.int())
    custodian(signal)

class TermHandler is SignalNotify
  let _env: Env
  let _custodian : Custodian

  new iso create(env: Env, custodian : Custodian) =>
    _env = env
    _custodian = custodian

  fun ref apply(count: U32): Bool =>
    _env.out.print("TERM signal received, shutting down")
    _custodian.dispose()
    true

In order to fix this behaviour under the PR, the signal handler can be created like so:

let signal = SignalHandler(TermHandler(env, custodian), Sig.int() where noisy = true)

mfelsche · 2018-04-05T08:02:26Z

That is indeed a nice change!

There is nonetheless one thing i would like to ask of you: Could you describe in a few words the behaviours when noisy is set to true or false in the docstring of the method? It is quite important as it has significant impact on the runtime behavior.

mfelsche · 2018-04-05T08:04:23Z

Also this section should be adapted: https://github.com/ponylang/ponyc/blob/master/packages/signals/sig.pony#L42

SeanTAllen · 2018-04-05T12:02:07Z

I understand the reasoning behind this but I think this needs further discussion. We need to verify that any noisy signal can be cancelled otherwise the program is unable to exit.

…

On Thu, Apr 5, 2018, 4:04 AM Matthias Wahl ***@***.***> wrote: Also this section should be adapted: https://github.com/ponylang/ponyc/blob/master/packages/signals/sig.pony#L42 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2631 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAL0PLvxEifme4MucT9sMyJYZiAiSaB7ks5tldAIgaJpZM4TH9rg> .

sargun · 2018-04-05T14:30:40Z

@SeanTAllen Cancelled by who, the user? If it helps, we can create a whitelist of signals that are allowed to be registered in a noisy way.

SeanTAllen · 2018-04-05T14:33:26Z

Yes cancelled by the user.

If its noisy then you need to be able to shut it down so that the program can exit if the signal is never received.

jemc · 2018-04-05T17:22:36Z

packages/signals/signal_handler.pony

@@ -15,14 +15,14 @@ actor SignalHandler
  let _sig: U32
  var _event: AsioEventID

-  new create(notify: SignalNotify iso, sig: U32) =>
+  new create(notify: SignalNotify iso, sig: U32, noisy : Bool = false) =>


Small formatting issue - can you remove the whitespace before the :, to make the code style match the style guide?

jemc · 2018-04-05T17:25:07Z

To cancel the noisy listener, I think dispose should already do the trick here, so no more change there should be needed I think (but I haven't verified).

I agree that finding a better name for this than noisy would be good (but currently not sure what to suggest). And I also agree that the consequences of the parameter need to be explained in a docstring for this method.

jemc · 2018-04-05T17:50:53Z

Maybe wait is a good name for the parameter, as long as it's explained in the docstring?

Something like:

If the wait parameter is true, the program will not terminate until dispose is called.

sargun · 2018-04-05T17:53:55Z

Noisy was really confusing for me as I was browsing the codebase. I wish there was a better term to describe what it meant.

…

On Thu, Apr 5, 2018 at 10:25 AM, Joe Eli McIlvain ***@***.***> wrote: To cancel the noisy listener, I *think* dispose should already do the trick here. I agree that finding a better name for this than noisy would be good (but currently not sure what to suggest). And I also agree that the consequences of the parameter need to be explained in a docstring for this method. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2631 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtyRORauqhz4vBIZBFnT1RPmbxH71LNks5tllN2gaJpZM4TH9rg> .

slfritchie · 2018-04-05T18:24:41Z

FWIW, my understanding of the word "noisy" is used in opposite of "quiescent" (which appears in scheduler.c). The runtime can shut down if all possible sources of messages are quiet (not sending anything) and will be quiet in the future.

jemc · 2018-04-05T18:52:13Z

Yeah, that's where "noisy" comes from, and it does make some sense after realizing the relationship to "quiescence" - it's just a bit of an odd / novel use of the word that I'm not aware of being used in any other programming contexts - perhaps because pony's model for detecting quiescence to terminate the program is also novel (as far as I know - I may be unaware of some prior art here).

SeanTAllen · 2018-04-05T19:47:19Z

I think noisy fine in the codebase when not exposed to folks via a Pony API. When I get around to creating a Pony API for the various Pony ASIO ffi methods, I'd want to use a different name there was well.

…

On Thu, Apr 5, 2018, 2:52 PM Joe Eli McIlvain ***@***.***> wrote: Yeah, that's where "noisy" comes from, and it does make some sense after realizing the relationship to "quiescence" - it's just a bit of an odd / novel use of the word that I'm not aware of being used in any other programming contexts - perhaps because pony's model for detecting quiescence to terminate the program is also a bit novel (as far as I know). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2631 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAL0PJ_hP1tsCK0G1-sQCXXr--jZhIAHks5tlmfegaJpZM4TH9rg> .

sargun · 2018-04-08T03:31:23Z

@jemc Changed to "wait"

jemc · 2018-04-09T22:32:53Z

packages/signals/signal_handler.pony

@@ -10,19 +10,22 @@ use @pony_asio_event_destroy[None](event: AsioEventID)
 actor SignalHandler
  """
  Listen for a specific signal.
+
+  If the wait parameter is true, the program will not terminate until the SignalHandler's dispose method is called.
+


This extra empty line isn't needed.

jemc

Looks good to me, other than the small empty line comment I made above.

Anybody else have things to say about this?

slfritchie · 2018-04-10T02:34:19Z

Fine by me. Should this have a changelog+added tag added?

sargun · 2018-04-10T04:02:09Z

@jemc Just removed the whitespace.

sargun · 2018-04-10T04:02:12Z

@jemc Just removed the whitespace.

mfelsche · 2018-04-10T07:05:14Z

packages/signals/signal_handler.pony

@@ -10,19 +10,21 @@ use @pony_asio_event_destroy[None](event: AsioEventID)
 actor SignalHandler
  """
  Listen for a specific signal.
+  If the wait parameter is true, the program will not terminate until the SignalHandler's dispose method is called.


i would also add: "or if the SignalNotify returns false, after handling the signal"
as this also disposes the SignalHandler and unsubscribes from asio.

I think that "logic" is already documented in SignalNotify.pony:

Called with the the number of times the signal has fired since this was last called. Return false to stop listening for the signal.

True point. I would add it nonetheless, as it is important for users to understand under which circumstances waiting stops. It is more convenient to users to have this information in one place.

Thank you for the PR, it looks very good.

This allows programs that are waiting on a signal to proceed to work. Sometimes, there are programs that are waiting for an alarm, or similar prior to performing I/O. This was impossible, because signal handlers were never registered as noisy actors in asio. This allows for an optional named parameter to be passed with that context.

sargun · 2018-04-10T18:34:12Z

@mfelsche Added a bit more info to the docstring

SeanTAllen · 2018-05-02T19:54:11Z

Thanks @sargun

jemc reviewed Apr 5, 2018

View reviewed changes

sargun force-pushed the noisy-signals branch from 752a2de to 520087f Compare April 7, 2018 15:19

jemc reviewed Apr 9, 2018

View reviewed changes

jemc approved these changes Apr 9, 2018

View reviewed changes

sargun force-pushed the noisy-signals branch from 520087f to 6fe019a Compare April 10, 2018 03:59

mfelsche reviewed Apr 10, 2018

View reviewed changes

sargun force-pushed the noisy-signals branch from 6fe019a to 95bb2d1 Compare April 10, 2018 16:19

mfelsche approved these changes Apr 10, 2018

View reviewed changes

SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label May 2, 2018

SeanTAllen merged commit 2f35bd0 into ponylang:master May 2, 2018

ponylang-main added a commit that referenced this pull request May 2, 2018

Update CHANGELOG for PR #2631 [skip ci]

081eec0

dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018

Update CHANGELOG for PR ponylang#2631 [skip ci]

534e23f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mechanism to register (noisy) signal handler #2631

Add mechanism to register (noisy) signal handler #2631

sargun commented Apr 5, 2018

mfelsche commented Apr 5, 2018

mfelsche commented Apr 5, 2018

SeanTAllen commented Apr 5, 2018 via email

sargun commented Apr 5, 2018

SeanTAllen commented Apr 5, 2018

jemc Apr 5, 2018

sargun Apr 7, 2018

jemc commented Apr 5, 2018 •

edited

Loading

jemc commented Apr 5, 2018

sargun commented Apr 5, 2018 via email

slfritchie commented Apr 5, 2018

jemc commented Apr 5, 2018 •

edited

Loading

SeanTAllen commented Apr 5, 2018 via email

sargun commented Apr 8, 2018

jemc Apr 9, 2018

jemc left a comment

slfritchie commented Apr 10, 2018

sargun commented Apr 10, 2018

sargun commented Apr 10, 2018

mfelsche Apr 10, 2018

sargun Apr 10, 2018

mfelsche Apr 10, 2018

sargun commented Apr 10, 2018

SeanTAllen commented May 2, 2018

Add mechanism to register (noisy) signal handler #2631

Add mechanism to register (noisy) signal handler #2631

Conversation

sargun commented Apr 5, 2018

mfelsche commented Apr 5, 2018

mfelsche commented Apr 5, 2018

SeanTAllen commented Apr 5, 2018 via email

sargun commented Apr 5, 2018

SeanTAllen commented Apr 5, 2018

jemc Apr 5, 2018

Choose a reason for hiding this comment

sargun Apr 7, 2018

Choose a reason for hiding this comment

jemc commented Apr 5, 2018 • edited Loading

jemc commented Apr 5, 2018

sargun commented Apr 5, 2018 via email

slfritchie commented Apr 5, 2018

jemc commented Apr 5, 2018 • edited Loading

SeanTAllen commented Apr 5, 2018 via email

sargun commented Apr 8, 2018

jemc Apr 9, 2018

Choose a reason for hiding this comment

jemc left a comment

Choose a reason for hiding this comment

slfritchie commented Apr 10, 2018

sargun commented Apr 10, 2018

sargun commented Apr 10, 2018

mfelsche Apr 10, 2018

Choose a reason for hiding this comment

sargun Apr 10, 2018

Choose a reason for hiding this comment

mfelsche Apr 10, 2018

Choose a reason for hiding this comment

sargun commented Apr 10, 2018

SeanTAllen commented May 2, 2018

jemc commented Apr 5, 2018 •

edited

Loading

jemc commented Apr 5, 2018 •

edited

Loading