Pretty errors

[FFY00]

On top of #53 to prevent conflicts.

Example output (ERROR is red, try it!):

Traceback (most recent call last):
  File "/home/anubis/git/ViewSB/viewsb/ipc.py", line 33, in run
    multiprocessing.Process.run(self)
  File "/usr/lib/python3.8/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/home/anubis/git/ViewSB/viewsb/ipc.py", line 157, in _subordinate_process_entry
    task = remote_class(**arguments)
  File "/home/anubis/git/ViewSB/viewsb/backends/usbmon.py", line 402, in __init__
    FileBackend.__init__(self, filename)
  File "/home/anubis/git/ViewSB/viewsb/backend.py", line 99, in __init__
    self.target_file = open(target_file, 'rb')
PermissionError: [Errno 13] Permission denied: '/dev/usbmon2'

ERROR [Errno 13] Permission denied: '/dev/usbmon2'

Similarly to the other PRs, please read the commit messages :blush:

[FFY00]

The traceback could be disabled, and only shown when a --debug flags is passed, or a VIEWSB_DEBUG env var is set, for eg. Let me know if you want to go that route.

[FFY00]

Forgot to say, this also fixes ViewSB not exiting on errors like the one in the example abose, I can split that patch (ipc: add vital argument to ProcessManager.start) into a separate PR if you want.

[FFY00]

Ugh, this actually gets messed up with the TUI frontend. I think the frontends should override ipc.handle_exceptions and have their custom implementation.

[FFY00]

Rebased on top of master.

[FFY00]

@Qyriad no pressure but I was wondering how is the progress reviewing this? Is there anything I could help with? We could have a chat via irc, discord, etc. if you are unsure about the design and implementation choices. This is a big PR so really no pressure :blush:

[Qyriad]

Thank you for the contribution; I really appreciate the code submission. I'm not sure I'm understanding the goal of this PR. Is it just to add the red ERROR at the bottom? If so, spinning up an independent thread seems like a lot. Is there an additional benefit to doing it this way?

[FFY00]

This PR adds a way for errors in IPC subprocesses to be handled, but currently I only use that in main to add the extra print (actually I didn't print the traceback at first but changed that later). The frontends could, and should, override this in favor of specific behavior. For eg. Qt could show the error in a pop-up, it could also provide some extra options other than just exiting, like to retry, open an issue, etc. The main goal with this is to be able to have better UX down the line.

The thread part is not actually required. I added the commit here but it could be split into a separate PR. Currently, ViewSB keeps running if a fatal error occurred in IPC subprocesses, like you not having enough permissions to open usbmon interfaces. These errors can be handled using the mechanism introduced above, but if we want the program to actually exit, we need to exit the parent process. Unfortunately there is no cross-platform way to do this in Python from the child process. The way I propose this to be fixed is to spin up a thread, which will be sleeping most of the time anyway, to monitor the vital IPC subprocesses, and if they exit with an error (non-zero) we will exit the main process with the same code.

Forgot to say, this also fixes ViewSB not exiting on errors like the one in the example abose, I can split that patch (ipc: add vital argument to ProcessManager.start) into a separate PR if you want.

Let me know if you need more clarification, there are a lot of moving parts, multiprocessing in Python is messy :sweat_smile:

You may also want to check out the commit messages if you haven't done that already.

[Qyriad]

Currently, ViewSB keeps running if a fatal error occurred in IPC subprocesses, like you not having enough permissions to open usbmon interfaces.

So, I believe this was actually partially fixed as part of #57. I talked about the details in https://github.com/usb-tools/ViewSB/issues/27#issuecomment-700612059:

The analyzer just wasn't periodically checking if the backend was still alive (like it was doing with the frontend). Once I fixed that (dc1a90b) the analyzer was correctly asking the frontend to stop, but the Qt frontend wasn't listening. Once I added a check for that in the Qt frontend's event loop (7e6b96e), the Qt frontend started correctly stopping if the backend stopped, as it should. So what this means is that now this is a matter of the frontends properly listening to the analyzer, and checking if the analyzer has asked it to stop (specifically, checking termination_event.is_set()). This was easy for me to implement in the Qt frontend (and automatic in the CLI frontend). I haven't yet implemented this for the TUI frontend, since I think that'll involve me having to manage the urwid event loop myself, which I haven't yet put the time into figuring out how to do.

Once we give the backend the ability to communicate with the analyzer, it could very easily tell the frontend what error occurred (which would be much more useful once the frontend can also e.g. tell the backend to retry or open a different file).

[FFY00]

Hum. Okay, I just need to know if IPC processes could be used in other contexts, and if we can assume all of those uses will have a way to communicate.

I am gonna open another PR with an implementation based on frontend <-> backend communication, instead of having an external entity managing it, and then we can see what makes most sense.

[FFY00]

Closing in favor of #61