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 f531d2c5d9530242a55be3f238c1372aa12b434f)
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.
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 callbackhas 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
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
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:
core::create(_simple)) are not safe to be deleted without locking thethread-loopThis means that one has to lock thethread-loopand 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.eventsthe callback for theeventwill be called from thethread-loops thread. This is really problematic because you can'tupdatefrom 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-loopis 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 methodcreate/bindthat returns atl::expectedand constructs the object - It would then return the exception when an error was thrown.However this would require
createto wait for the object to be successfully bound/created to know if it has an error (i.e.updatethe core, which in turn would also complicate things a little)(implemented in f531d2c5d9530242a55be3f238c1372aa12b434f)
Possible Solutions
1. Revert the
thread-loopchanges and bring backcall_safe.Pros:
eventsissue.Cons:
call_safehad it's own caveats and things I didn't like about it2. Make object constructors (i.e.
node,port, ...) private and use static method to instantiate the objectAnd return
shared_ptrthat has custom deleter to make sure object is deleted safely.Pros:
Cons:
eventsissue3. The
pipewire-rsappraochDesign:
Channelnode,device,proxy) we throw a compile-time error (in case the passed object is not aconst-referenceorborrow_ptr) to make sure the passed object stays in the loop threadRequired changes to current code
main-loopthey get bound to was initialized on the same thread.main_loop::run()should assert that the creation thread is also the execution threadPros:
eventsissueCons:
4. Design all inherently unsafe methods to be async
Design:
thread_loop) lock the core and bind / create the object, then unlock it and return the not yet evaluated resulteventmain_loop) bind / create the object, trigger anupdateand return the promisepromise::get(),promise::wait()assertif called from within thethread-loops thread.promise::error()that takes a callback which is invoked in case of an errorpromise::then()that takes a callback which is invoked once the result is readystatewhich will live until thecallbackhas been calledthread-loops thread (as long as the object is not movable, copyable, ...)thread-loops threadPros:
call_safemechanism had (namely that the object is guaranteed to die within thethread-loops thread)call_safeapproach, as all functions that would require special care would return a promiseCons:
call_safe) it's still easy to do things wrong by accident💡 Help is always appreciated!
Current proposed solution
After some back and forth I think I will settle with solution 3