Curve/rohrkabel

Thread-Loop Improvements

Curve opened this issue · 0 comments

Curve commented

Rohrkabel needs some heavy improvements regarding the Thread-Loop.

The current support for the thread-loop is a bit unintuitive, namely the following things are not as clear and simple as I'd like them to be:

  • Thread Loop
    • All created objects (through core::create(_simple)) are not safe to be deleted without locking the thread-loop
      This means that one has to lock the thread-loop and then explicitly delete the created object
      This is a big caveat and can cause problems when a user is not aware of this, also it's really ugly.
    • When using any form of events the callback for the event will be called from the thread-loops thread.
      This is really problematic because you can't update from within it.
  • Exceptions
    Generally, I'd like to avoid throwing exceptions where possible. Currently we throw on pw_proxy_events::error.
    This is problematic when the thread-loop is used because the exception can then not be properly caught.

    I'd like to replace the exceptions, maybe we could make the constructor of proxy, node, etc. private and have a static method create / bind that returns a tl::expected and constructs the object - It would then return the exception when an error was thrown.
    However this would require create to wait for the object to be successfully bound/created to know if it has an error (i.e. update the core, which in turn would also complicate things a little)

(implemented in f531d2c)

Possible Solutions

1. Revert the thread-loop changes and bring back call_safe.

Pros:

  • Would circumvent the events issue.

Cons:

  • call_safe had it's own caveats and things I didn't like about it
  • Not as elegant as I'd like it to be

2. Make object constructors (i.e. node, port, ...) private and use static method to instantiate the object

And return shared_ptr that has custom deleter to make sure object is deleted safely.

Pros:

  • Would solve the destruction thread-safety issue
  • Is somewhat elegant

Cons:

  • Would not solve the events issue

3. The pipewire-rs appraoch

See https://pipewire.pages.freedesktop.org/pipewire-rs/pipewire/channel/index.html#examples

Design:

  • Channel

    • Can send and receive messages to/from the main-loop
      • The exchanged messages can be of any arbitrary type
        • In case the channel tries to send a pipewire-object (i.e. node, device, proxy) we throw a compile-time error (in case the passed object is not a const-reference or borrow_ptr) to make sure the passed object stays in the loop thread
  • Required changes to current code

    • Core, Context, Registry should assert that the main-loop they get bound to was initialized on the same thread.
    • main_loop::run() should assert that the creation thread is also the execution thread

Pros:

  • Is pretty elegant
  • Solves the events issue
  • Would have no additional impact on the current implementation

Cons:

  • None

4. Design all inherently unsafe methods to be async

Design:

  • All unsafe methods will ...
    • ... (in case the loop is a thread_loop) lock the core and bind / create the object, then unlock it and return the not yet evaluated result
      • we can not wait for the object to be evaluated here because it would not work when called from an event
    • ... (in case the loop is a main_loop) bind / create the object, trigger an update and return the promise
  • The promise can be waited upon (i.e. promise::get(), promise::wait()
    • However it would assert if called from within the thread-loops thread.
  • The promise has a method promise::error() that takes a callback which is invoked in case of an error
  • The promise has a method promise::then() that takes a callback which is invoked once the result is ready
    • The promise will have a state which will live until the callback has been called
    • This would ensure that the objects destructor is called from within the thread-loops thread (as long as the object is not movable, copyable, ...)
    • This would be safe to use even from within the thread-loops thread

Pros:

  • inherently solves some issues the call_safe mechanism had (namely that the object is guaranteed to die within the thread-loops thread)
  • may be a little more intuitive than the call_safe approach, as all functions that would require special care would return a promise

Cons:

  • Callback Hell
  • More complex and while it's a little harder to mess things up with this approach (in comparison to call_safe) it's still easy to do things wrong by accident
  • Will make the code way more complex depending on how it's implemented (would best be implemented with templates, which will in turn bring caveats when used on all major classes)
  • Also not the most elegant solution

This issue currently serves as an outlet for my ideas regarding this issue.
I will keep updating this post with ideas that come to mind and will leave it open until I have found a solution that I'm pleased with.

💡 Help is always appreciated!

In case you're reading this issue and have any ideas or suggestions on how to tackle this problem, feel free to comment!

Current proposed solution

After some back and forth I think I will settle with solution 3