Handle dispatch failure

[acroz]

This PR restarts the parser finite state machine if it exits with an exception. Prior to this change, failures in the handler would leave the stream in an unusable state, since calling .send() on the parser finite state machine would raise StopIteration.

[superbobry]

Thanks for another contribution!

I'm a bit cautious about adding any exception handling to Stream because of performance concerns (although this is for the benchmarks to confirm). In general, neither Stream nor Screen are not supposed to raise unless the input is corrupted in some way (or there's a bug in pyte). If this happens

• during parsing, the stream could lose some of the accumulated input
• during even dispatch, the screen could end up in a semi-broken state.

With this in mind, I have a counter proposal: wdyt about just adding a reset to Stream? This way the users would be able to reset it, but Stream itself would not do any implicit automagic.

Also, do you have examples of real failures you want to mitigate by this PR?

[acroz]

Source of the problem

We discovered that when exiting a particular program (midnight commander), the control sequence 'CSI ? Pm r' (where Pm is a semicolon-delimited set of digits, see http://invisible-island.net/xterm/ctlseqs/ctlseqs.html) was being read from the terminal process and written into the pyte Stream.

The current implementation of the CSI dispatcher in the streams module assumes that the meaning of the message can be uniquely determined from the last character of the control sequence. In this case, the character r is determined to indicate DECSTBM i.e. set the scrolling region. This gets mapped to the set_margins method of Stream, which does not accept a private argument as DECSTBM does not come with a ? after the CSI.

However, 'CSI ? Pm r' is a valid sequence, indicating "Restore DEC Private Mode Values". pyte matches this with the set_margins() method as described above, and tries to call it with a private=True argument, which fails as it is not an expected argument.

This causes a TypeError to be raised, which is bubbled out to the caller of feed(), but it also causes the Stream._parser generator to exit, causing further calls to feed() to fail with StopIteration, even if the TypeError was caught. Here is a snippet demonstrating this:

import pyte

screen = pyte.Screen(10, 6)
stream = pyte.Stream()
stream.attach(screen)

# Set mode 2
stream.feed('\x1b[2h')

# Restore DEC private mode values
try:
    stream.feed('\x1b[?10r')
except Exception as e:
    print('Caught exception:')
    print(repr(e))

# Set mode 4
print('Trying to feed another control sequence:')
stream.feed('\x1b[4h')

Solution

I considered extending the CSI parser to disambiguate messages on the basis that they are private or not, and we should perhaps still do that, however we don't really care about handling the "Restore DEC private mode values" message - we just want the parser to keep working.

That the parser is implemented as a generator (a very nice design btw) is an implementation detail that I don't think the caller should require knowledge of. There could be other exceptions raised inside the listening screen's methods that could also crash the parser. The motivation of this PR was therefore to continue to bubble out exceptions raised when handling messages, while allowing the user to catch and ignore those messages, but continue to call feed() as before.

In response to your concerns:

• As the parser is an implementation detail and not exposed outside of the Stream class, I think restarting it on failure is not really automagic.
• You were concerned about losing accumulated input - at present, all input is lost as the parser crashes.
• I didn't really understand your 'during even dispatch' message - can you elaborate?
• I appreciate the intention that both Stream and Screen are never supposed to fail, however considering the wide variety of messages terminals can send, some of which are not currently handled by pyte, it seems pragmatic to expected that failures might happen sometimes. In this case I think having the Stream keeping itself in a usable state seems sensible to me.

Other implementations I considered were:

• Create the parse lazily (initialize as None and overwrite with the actual parser on first use), and reset the parser to None when it exits with an exception, thereby forcing a new one to be created on next use.
• Use a try/except block to attempt to pass characters to the parser, and if a StopIteration is raised, create a new parser and try feeding the same character into it a second time. I find this less elegant, but it has the advantage of only recreating the parser if the generator is definitely broken (though I'm confident that it's broken when it exits with an exception anyway).

If you think one of these is better I'm happy to modify my PR accordingly.

[uSpike]

Hi, is there desire from the maintainers to merge these changes? It is affecting me since we are interfacing with SBCs and <100% reliable serial connections that drop characters from time-to-time.

[acroz]

I am happy to pick this up again if it helps, @superbobry

[superbobry]

Thanks everyone! I've merged the PR manually.