Document fiber user signals

[pnelson]

fiber/status may return :user(0-7) to indicate that the fiber is suspended by a user signal.

defer will also run form if a user signal 0-4 is received, I assume within the fiber it is being run in.

The signal function appears to be used to send user signals. The function documentation should probably make mention of "user" but I think the example provided on Zulip would make a great addition to the Fiber Overview page:

(defer (pp :hello) (signal :user1 :smile) (pp :oops))
:hello
user1: smile
  in signal [src/core/corelib.c] on line 701
  in defer [repl] on line 2, column 20
  in thunk [repl] (tail call) on line 2, column 1

[sogaiu]

I've started going over some of the related website docs and docstrings.

It does seem sensible to me to add mention of signal to the current version of the Fiber Overview page as it doesn't appear to be there currently, possibly with some example code as you suggested.

Regarding the function docs for signal, its current definition is:

JANET_CORE_FN(janet_core_signal,
              "(signal what x)",
              "Raise a signal with payload x. ") {
    janet_arity(argc, 1, 2);
    Janet payload = argc == 2 ? argv[1] : janet_wrap_nil();
    if (janet_checkint(argv[0])) {
        int32_t s = janet_unwrap_integer(argv[0]);
        if (s < 0 || s > 9) {
            janet_panicf("expected user signal between 0 and 9, got %d", s);
        }
        janet_signalv(JANET_SIGNAL_USER0 + s, payload);
    } else {
        JanetKeyword kw = janet_getkeyword(argv, 0);
        for (unsigned i = 0; i < sizeof(janet_signal_names) / sizeof(char *); i++) {
            if (!janet_cstrcmp(kw, janet_signal_names[i])) {
                janet_signalv((JanetSignal) i, payload);
            }
        }
    }
    janet_panicf("unknown signal %v", argv[0]);
}

My impression of that code is that the intention is for one to pass an integer or a keyword to signal for the value of what.

If it's an integer, the value is checked to be between 0 and 9 inclusive. If it's a keyword, I think it's checked against this list:

const char *const janet_signal_names[14] = {
    "ok",
    "error",
    "debug",
    "yield",
    "user0",
    "user1",
    "user2",
    "user3",
    "user4",
    "user5",
    "user6",
    "user7",
    "interrupt",
    "await"
};

If either of the checks pass then a call is made to janet_signalv.

I don't see these names listed in docstrings, though there are close ones in fiber/status:

    * :dead - the fiber has finished
    * :error - the fiber has errored out
    * :debug - the fiber is suspended in debug mode
    * :pending - the fiber has been yielded
    * :user(0-7) - the fiber is suspended by a user signal
    * :interrupted - the fiber was interrupted
    * :suspended - the fiber is waiting to be resumed by the scheduler
    * :alive - the fiber is currently running and cannot be resumed
    * :new - the fiber has just been created and not yet run

Perhaps it makes sense to add an appropriate list to signal's docstring as well as to explain the what parameter a bit. It that seems appropriate, I'll make an issue over at the janet repository.

[sogaiu]

A drawback to putting the list in signal's docstring is that now the information lives in two places with no obvious mechanism for keeping things in sync. May be a comment can be added above here as a reminder.

fiber/status already essentially duplicates the list here, so possibly that could also have a reminder added above it.

Come to think of it, I think I've seen a few reminders in janet's codebase sort of along these lines already.

May be these kinds of changes could count as an overall improvement on the current situation.

[pnelson]

Anything with "user" in signal documentation would have guided me. Something like "Raise a signal with payload x. See user signals in Fiber Overview."

Or the reverse, from fiber/status, ":user(0-7) - the fiber is suspended by a user signal, see signal".

[pnelson]

In retrospect, I probably could have seen "user signal" and been hinted towards signal. Would be more obvious with backticks or other visual queue though.

[sogaiu]

In general, I agree that the visual cue situation in docstrings could be improved (if multiple are located in a short period, likely PRs are welcome if done in batch). Though if you notice any and just mention them, someone might get motivated to do something about them :)

The following is my draft diff for improving signal's docstring as well as putting reminders in janet's source:

diff --git a/src/core/corelib.c b/src/core/corelib.c
index 7ac7f564..ee4723b6 100644
--- a/src/core/corelib.c
+++ b/src/core/corelib.c
@@ -700,7 +700,15 @@ JANET_CORE_FN(janet_core_is_lengthable,
 
 JANET_CORE_FN(janet_core_signal,
               "(signal what x)",
-              "Raise a signal with payload x. ") {
+              "Raise a signal with payload x. `what` can be an integer\n"
+              "from 0 through 7 indicating user(0-7), or one of:\n\n"
+              "* :ok\n"
+              "* :error\n"
+              "* :debug\n"
+              "* :yield\n"
+              "* :user(0-7)\n"
+              "* :interrupt\n"
+              "* :await") {
     janet_arity(argc, 1, 2);
     Janet payload = argc == 2 ? argv[1] : janet_wrap_nil();
     if (janet_checkint(argv[0])) {
diff --git a/src/core/util.c b/src/core/util.c
index 6cb5676e..a95f43af 100644
--- a/src/core/util.c
+++ b/src/core/util.c
@@ -79,6 +79,7 @@ const char *const janet_type_names[16] = {
     "pointer"
 };
 
+/* Docstring for signal lists these */
 const char *const janet_signal_names[14] = {
     "ok",
     "error",
@@ -96,6 +97,7 @@ const char *const janet_signal_names[14] = {
     "await"
 };
 
+/* Docstring for fiber/status lists these */
 const char *const janet_status_names[16] = {
     "dead",
     "error",

Apart from this, I'm thinking of suggesting changes to the Fiber Overview page that try to address some of the concerns mentioned in this issue. Hope to come up with some approriate draft (^^;

(I'm not really a fan of explicitly mentioning the website docs in docstrings though (although this is done for at least the special forms), but I do consider both the docstrings and website docs to be essential.)

[sogaiu]

Here's a draft diff for the Fiber Overview page:

diff --git a/content/docs/fibers/index.mdz b/content/docs/fibers/index.mdz
index 99bd41e..bc995e8 100644
--- a/content/docs/fibers/index.mdz
+++ b/content/docs/fibers/index.mdz
@@ -102,3 +102,20 @@ an error clause is evaluated.
  ([err] (print "oops")))
 # Evaluates to 6 - no error thrown
 ```)
+
+The @code[signal] function can be used to raise a particular signal.
+The function's first argument, @code[what], specifies the signal,
+while the second argument, @code[x], is an arbitrary payload value.
+
+@codeblock[janet](```
+(try
+  (signal :error 1)
+  ([err] (print "got error: " err)))
+# evaluates to nil and prints "got error: 1"
+
+(defer (pp :hey)
+  (signal :user4 :hello)
+  (pp :unreached))
+# only prints :hey
+```)
+

[sogaiu]

@pnelson I think both PRs that were made as steps to addressing this have been merged.

If you think the issue has been addressed may be the issue can be closed?

[pnelson]

Looks good! Thanks!