bridge: Report PID for a spawn stream channel

[martinpitt]

Add the spawned PID to the channel's "ready" message. This will be
useful for #21021

---

The first commit is unrelated for this PR, but you'll need it for the demo below. But having "once" event handlers is generally useful, so let's already stop typescript to yell at us about that.

@ashley-cui you can test it with this demo, that'll show the pid when opening the terminal, and it goes away when you control-d it.

diff --git pkg/systemd/terminal.jsx pkg/systemd/terminal.jsx
index 03ada383b..31de7e757 100644
--- pkg/systemd/terminal.jsx
+++ pkg/systemd/terminal.jsx
@@ -27,7 +27,7 @@ const _ = cockpit.gettext;
      */
     class UserTerminal extends React.Component {
         createChannel(user) {
-            return cockpit.channel({
+            const ch = cockpit.channel({
                 payload: "stream",
                 spawn: [user.shell || "/bin/bash"],
                 environ: [
@@ -37,6 +37,9 @@ const _ = cockpit.gettext;
                 pty: true,
                 binary: true,
             });
+            ch.addEventListener("ready", (_, msg) => this.setState({ pid: msg.pid }), { once: true });
+            ch.addEventListener("close", () => this.setState({ pid: null }), { once: true });
+            return ch;
         }
 
         constructor(props) {
@@ -66,6 +69,7 @@ const _ = cockpit.gettext;
                 title: 'Terminal',
                 theme: theme || "black-theme",
                 size: parseInt(size) || 16,
+                pid: null,
             };
             this.onTitleChanged = this.onTitleChanged.bind(this);
             this.onResetClick = this.onResetClick.bind(this);
@@ -183,6 +187,9 @@ const _ = cockpit.gettext;
                                             className="pf-v5-c-button pf-m-secondary terminal-reset"
                                             onClick={this.onResetClick}>{_("Reset")}</button>
                                 </ToolbarItem>
+                                <ToolbarItem>
+                                    <span>shell pid: {this.state.pid}</span>
+                                </ToolbarItem>
                             </ToolbarContent>
                         </Toolbar>
                     </div>

[martinpitt]

For posterity, a cockpit.js API could look like this:

--- pkg/lib/cockpit.js
+++ pkg/lib/cockpit.js
@@ -1447,9 +1447,12 @@ function factory() {
         /* Callback that wants a stream response, see below */
         const buffer = channel.buffer(null);
 
+        channel.addEventListener("ready", (_, message) => { dfd.promise.pid = message.pid });
+
         channel.addEventListener("close", function(event, options) {
             const data = buffer.squash();
             spawn_debug("process closed:", JSON.stringify(options));
+            dfd.promise.pid = undefined;
             if (data)
                 spawn_debug("process output:", data);
             if (options.message !== undefined)

But its awkward -- it slaps custom stuff on the cockpit promise, and we want to get rid of these. It's also difficult to test/use, as a cockpit.spawn() object neither has .wait() nor .addEventListener("ready", ...), so there is no race free way (other than polling) to actually get the pid. I got stuck with writing a unit test for that, and then decided it's not worth it for now.

[martinpitt]

@allisonkarlitskaya pinging you as the pybridge is close to your heart. But please absolutely feel free to ignore this review request.

[mvollmer]

But its awkward -- it slaps custom stuff on the cockpit promise, and we want to get rid of these.

Yeah, let's make a new API proc = cockpit.run(...) that does not return a promise directly, but some object with methods like const [code, stdout, stderr] = await proc.wait() that returns a promise. Or even proc.communicate! There could be pid = await proc.pid().

[mvollmer]

I think this is wrong. Our addEventListener implementation does not support the options parameter. Calling it with one should be an error.

[martinpitt]

Oh, TIL -- this is not the standard EventSource, but our custom pkg/lib/cockpit/_internal/event-mixin.js via pkg/lib/cockpit/_internal/channel.js. Eww! I took out that commit then.

@ashley-cui so, my example code above "silently" works, but is buggy and leaks event handlers. It needs to store the result of addEventListener(), and explicitly call removeEventListener() on them on close. (but that's details which we can sort out in the PR)

[mvollmer]

It needs to store the result of addEventListener(), and explicitly call removeEventListener() on them on close.

Aren't event handlers removed by the GC when the channel object is collected? We don't remove event handlers from our channel objects explicitly in general.

If you want a guarantee that your handler is only called once, I think you are set as well: "ready" and "close" are only emitted once.

[martinpitt]

Aren't event handlers removed by the GC when the channel object is collected?

Perhaps/I hope so, I'm just never sure when exactly that happens. It's not synchronous like in C, Vala or Rust, AFAIK there's an actual GC running (similar to Python). This doesn't matter for this case, but it seems easy to have event handlers like systemd signals or D-Bus "PropertiesChanged" which wreak havoc when they happen multiple times and call setState().

[mvollmer]

Thanks!