greatscottgadgets / ViewSB

[Archived] open-source USB analyzer toolkit with support for a variety of capture hardware
BSD 3-Clause "New" or "Revised" License
343 stars 59 forks source link

Allow frontends to handle backend exceptions #61

Closed FFY00 closed 4 years ago

FFY00 commented 4 years ago

As discussed in #56. This PR makes it so that the frontend can handle the backend exceptions. Check the commit descriptions for more details.

FFY00 commented 4 years ago

@Qyriad do you have any feedback on this approach? No worries if you are busy, I can wait :blush:

As always, I am available to go over this and discuss if this is indeed the best approach.

Qyriad commented 4 years ago

Right, thank you for pinging me on this (and thank you again for all your contributions!)

So, there is something that concerns me that I didn't realize before. I'm definitely not a master on Python internals or multiprocessing, but I'm worried that near-universally trapping subprocess exceptions and then sending the objects themselves across pipes—far away from their original context—could cause unexpected strangeness. A case I can think about off the top of my head is debuggers using exceptions to break. If the exception is trapped and then manually printed in handle_exception(), will the debugger catch it? I doubt it, though admittedly I haven't gotten around to testing this specific case yet, but I'm just very unsure how Fancy exception objects are, and how much utility we might lose by pulling them out of their context. That's not to say I'm entirely against this approach, and in general I do think Something is better than Nothing—even if this may all be overhauled as, ideally, in the future the frontend should probably choose, start, and manage the backend.

FFY00 commented 4 years ago

So, there is something that concerns me that I didn't realize before. I'm definitely not a master on Python internals or multiprocessing, but I'm worried that near-universally trapping subprocess exceptions and then sending the objects themselves across pipes—far away from their original context—could cause unexpected strangeness.

Me neither. I don't think that would happen. It may seem that the exception implementation is somewhat magical, but I don't think it is that involved. I am really pretty sure there are no invisible context ties, the exception is just an object, so we are free to send it elsewhere. The missing context is provided by the traceback, from which we get a string representation and send it together with the exception object.

A case I can think about off the top of my head is debuggers using exceptions to break. If the exception is trapped and then manually printed in handle_exception(), will the debugger catch it?

As long as you register it in process callable (target), it should. We only catch it the after it exits target.

I doubt it, though admittedly I haven't gotten around to testing this specific case yet, but I'm just very unsure how Fancy exception objects are, and how much utility we might lose by pulling them out of their context.

As I said above, I don't think they are fancy at all. The exception objects should be just normal objects, they should not depend on anything externally. The context is provided by the traceback, and the way we handle that is by getting a string representation, which is good enough for our use-case. I have actually never needed to acess anything other than the string representation of a traceback, so I'd say it's pretty rare needing anything more.

That's not to say I'm entirely against this approach, and in general I do think Something is better than Nothing—even if this may all be overhauled as, ideally, in the future the frontend should probably choose, start, and manage the backend.

Yeah, think the architecture could be restructured a bit.

Qyriad commented 4 years ago

Alright, then with the changes I requested I'm alright with merging this, though if/when the architecture changes this may end up being significantly altered.

Thank you for the contribution ^^

FFY00 commented 4 years ago

I ended up renaming the pipes to _pipe_{send,recv}_{backend,frontend}_exception so that we can use the same scheme for all. Let me know if this is satisfactory for you or if I should rename them :sweat_smile:

FFY00 commented 4 years ago

@Qyriad friendly ping :blush: