Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Have you considered catch_unwind() at FFI boundaries? #122

Open
helgoboss opened this issue Apr 4, 2020 · 6 comments
Open

Have you considered catch_unwind() at FFI boundaries? #122

helgoboss opened this issue Apr 4, 2020 · 6 comments

Comments

@helgoboss
Copy link
Contributor

Right now, if the host calls into a vst-rs plugin and the plugin panics because of a bug, the host crashes (at least if the plugin runs in the same process like the host). Fail-fast is a good thing, but a crash not really. So if it can be avoided, I think it should, at least in production builds.

For a good amount of panics this could be avoided by wrapping the executed code in std::panic::catch_unwind(). Of course this could be done by the vst-rs user in the Plugin class methods, but maybe it would be better to do it directly at the FFI boundary, that means in vst-rs source code. Have you considered this?

I'm aware that doing this would require an alternative form of making the user aware of that error so that it doesn't go unnoticed. What do you think?

@Boscop
Copy link
Member

Boscop commented Apr 5, 2020

Nice to hear from you. I'm a big fan of your ReaLearn VST and wrote a similar VST in Rust, but since I have hundreds of mappings that are auto generated, my VST loads them from a config file instead. But I couldn't get my VST registered as a control surface for some reason (for getting notified of track / fx changes).
(That reminds me, I wanted to open source my Reaper API wrapper / loader.)

Regarding this issue, I think it makes sense to catch panics, especially in release builds. But do you think it would be acceptable if a panic happens, to not tell the plugin, to just bypass it in every method? (So that it behaves like a "default" plugin with pass-through audio, no params, no GUI etc.)

Because if a panic happens, it's usually unrecoverable. If it was supposed to be handled, it would be handled before turning into a panic.

@helgoboss
Copy link
Contributor Author

Hello Boscop. Oh great, happy to hear that! It's funny you mention ReaLearn VST because this is exactly the reason why I wrote this issue. I'm in the process of porting ReaLearn to Rust (for several reasons) and this time it will be open source. By the way, under the hood it uses several crates, one of them is reaper-rs (a port of ReaPlus, which is one of the basic building blocks of ReaLearn). reaper-rs is a REAPER API wrapper as well, soon to be released. It has control surface support, too.

Because if a panic happens, it's usually unrecoverable. If it was supposed to be handled, it would be handled before turning into a panic.

Yes, I'm referring to unrecoverable situations only.

Regarding this issue, I think it makes sense to catch panics, especially in release builds. But do you think it would be acceptable if a panic happens, to not tell the plugin, to just bypass it in every method? (So that it behaves like a "default" plugin with pass-through audio, no params, no GUI etc.)

Good question. I think it depends on the thread in which the panic occurs. If it happens in the main thread, I think it would be good to display a message box or something to make the user aware that something went wrong, without crashing the whole host (reaper-rs notifies the user by writing something to the REAPER console ... which we don't have though in case of a generic VST plugin). The audio processing could even continue in this case.

If it's in the audio thread, bypassing would be acceptable in my opinion. And again, the UI thread could be informed so that it notifies the user about the issue.

@Boscop
Copy link
Member

Boscop commented Apr 6, 2020

Nice, I'm looking forward to see how I can register my VST as a control surface in Reaper..
Yeah, in case of a panic, we could show that message box instead of the VST's GUI.
And since we're already using the log crate, we could log a stacktrace automatically for debugging (I'm usually using the log-panics crate in VSTs also, and logging to a file in the same dir as the dll (using get_folder_path to get the folder of the VST dll)).
If the panic didn't happen in the audio thread, the plugin could continue processing audio & midi. But if the panic happened in the audio thread, the GUI would also be replaced by the message box..

@helgoboss
Copy link
Contributor Author

Sounds good. Should I make a PR or do you want to give it a go? I wonder what the other vst-rs devs think about that. Introducing catch_unwind in itself is probably not that controversial, but I imagine the way how to react to it (message box and/or log file and/or X) might be a topic for more discussion, considering that vst-rs is a very generic library. I personally think there should be a sensible default behavior (exactly the one you described) but also the possibility to customize it. Latter could be done quite easily by overwriting the panic hook.

@Boscop
Copy link
Member

Boscop commented Apr 6, 2020

Yeah, feel free to give it a go.
(I'm currently quite busy with life etc.)
Btw, we also have a discord server where we sometimes discuss such things.
(Originally we started on this telegram channel which is still active.)
And we have a discourse: https://rust-audio.discourse.group/

@helgoboss
Copy link
Contributor Author

@Boscop Sorry for hijacking the issue, but since you mentioned your interest: reaper-rs is now available on GitHub. Hope it helps you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants