hooks for custom control sequences (updated)

[PerBothner]

This is pull request #1813 is a separate branch as request.
That pull requst should probably be closed as a duplicate or something.
This pull request is still unnecessarily messy, as it includes commits that are just reverts of older commits. The actual real commits are 8ceea11 5af4626 8a5a032 6b65ebd .

Fixes #1176

[jerch]

👍 LGTM. Lets wait for Tyriars response though.

Sorry Per, have to dismiss the review - imho there are still issues with the dispose function.

[jerch]

Looks like there are 2 possible memory leaks.

[jerch]

nextHandler gets not freed anywhere possibly holding that handler forever (if the disposed handler gets not removed on caller side). Same with the handlers object. Imho nulling both afterwards and conditionally exiting at the beginning when they not exist anymore (already disposed) solves this.

[PerBothner]

"nextHandler gets not freed anywhere possibly holding that handler forever (if the disposed handler gets not removed on caller side)."

Right - but wouldn't that be a leak on the caller side? You really shouldn't keep a reference tofoo after calling foo.dispose(). We can null out nextHandler as a precaution, but it would only serve to paper over a leak in the caller, methinks:

        if (cur === newHead) {
          if (previous) { previous.nextHandler = cur.nextHandler; }
          else { handlers[index] = cur.nextHandler; }
+         cur.nextHandler = null;
          break;
        }

"Same with the handlers object."

I don't think that is an issue. The handlers object is this._csiHandlers or this._oscHandlers, which are both allocated in the constructor.

[jerch]

Yepp thats basically a leak on caller side, but in JS ppl tend not to think about lifetime of an object at all. And we cannot control caller side here as its exposed to the public. Therefore I think we should take care over what we can control as a precaution. Both refs are likely to contain handlers of InputHandler which itself contains a Terminal ref. Terminal is kinda the root of our "disposable object tree", thus forgetting to remove a disposed handler ref might prevent the whole object tree from GCing, which is far worse than just a leftover handler ref.

Now talking about dispose and GC - I think it is also flawed regarding the invocation on a higher tree object that takes care of the subtree objects (the default dispose descent). I think EscapeSequenceParser.dispose also would have to invoke dispose on the added handlers (with nulling in place), otherwise InputHandler will be kept bound in the added handlers. (InputHandler.dispose itself nulls the terminal object, so at least the majority of objects will be released.)

[jerch]

Additional note on that: From API perspective its a surprising side effect, that an added handler pulls in refs to the terminal object (through handlers and nextHandler). Its not obvious for ppl, they only see the interface that acts on parser states and input. Atm dispose covers the "remove" thing, but not the ref releasing.

Btw the arrow function as added handler binds the parser object as this, just to be able to call the fallback. Hmm. Thats not the case for handlers via set.... Imho this needs a slightly different approach.

[PerBothner]

The only way I can see a leak that we can avoid is if there are two handlers A and B for the same index, with A added first, such that B.nextHandler === A. The caller calls B.dispose(), while still keeping a reference to B. Then even if A.dispose() is called later or dispose is called on the EscapeSequenceParser, there will still be a dangling reference to A, even if the caller has properly called dispose on it and no longer has a reference to it. I.e. a leak in whoever added B also causes A to leak, even though whoever added A is not at fault.

I'm not convinced this is a likely scenario, but it seems a good idea to null the nextHandler, regardless.

"that an added handler pulls in refs to the terminal object (through handlers and nextHandler)"

I don't see where the handler itself has a ref to the terminal object (unless the EscapeSequenceParser has a ref). However, the caller-supplied function is likely to have a reference.

I added a commit 48ff841 for "just-in-case" cleanup. Is that ok?

[Tyriar]

I created PerBothner#1 with some proposed changes:

• Moved to arrays instead of linked lists for managing
• Strongly typed _*Handlers
• Simplified dispose logic (also the old dispose didn't work)
• Added some thorough tests

On leaking memory, I think the nulling out that's in that PR should be sufficient, in the future we will probably do that less and less as it's a pain to deal with T | null when strict null checks are on and we will likely just rely on the garbage collector to free things properly. It's up to embedders to free their references to the terminal, disposables, etc.

[PerBothner]

I have no objection to using arrays instead of linked list. That is certainly simpler and more readable. The downside is it might use slightly more memory (an extra array per handler index) and be slightly slower (especially in the simple cases when "add" hasn't been called). If you and jerch are happy, I'm happy.

(I also have two more specific comments at PerBothner#1.)

[Tyriar]

LGTM, @jerch could you give it a final review/sign off too? We can merge this into 3.10 as it's pretty low risk. I added "Fixes #1176" to close that issue, if people need hooks for other types we'll get requests.

[jerch]

The dispose chain looks clean to me now (beside the handler array, but thats no biggie imho as it does not hold refs to the parser or any other object up to terminal).
Almost ready to ship, just the handler removal does not yet do the right thing imho.

[jerch]

@PerBothner Thx alot, hope it was not to bumpy to get it done.

With this we have a solid base to custom CSI and OSC functionality through addons. Still not sure how to deal with terminal internals if needed by addons, but I think we can decide that when really needed by a custom addon.

[Tyriar]

@PerBothner thanks for bearing with us 😃

[PerBothner]

You're welcome. I tend to be a bit of a perfectionist myself ...

[thesoftwarephilosopher]

How do you calculate the string version of intermediates based on the number version in params? I'm trying to use addCsiHandler to capture ?1049h and ?1049l (since I can't find a onToggledAltScreen event anywhere) and I get [1049] in the params but I can't figure out what intermediates is supposed to be to capture this specifically. I know I can just omit that and check if params[0] === 1049 which is what I'm doing now, but it'd be cleaner and probably more efficient to just check the one case; plus I already went down this rabbit hole...

[jerch]

@sdegutis It works the way around - you have to register a hook that correctly matches the prefix, intermediates and final of the sequence, example:

// hook into DECSET - defined as CSI ? P1;P2;...;Pn h
addCsiHandler({prefix: '?', final: 'h'}, params => {
  // params contains [P1, P2, ..., Pn]
});

DECSET in particular has no intermediates. In general a CSI sequence follows this scheme:

CSI <prefix> <params> <intermediates> <final>

Every value has byte restrictions to get parsed properly, see docs.

Edit: Ok seems you are looking for a hook for a specific param value an CSI? Thats not possible, we do not split the sequences further, thus you have to iterate through params and handle the one you are interested in. Dont forget to return false to keep the default handler running (as its not a good idea to skip the default handler here).