New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FL-3783] Asynchronous Infrared remote manipulation #3503
Conversation
Compiled f7 firmware for commit |
What happens to the concurrent jobs if the user leaves the scenes triggering them very fast? Seems like it's impossible to block on the runner task to wait before freeing the context data gracefully, which seems like it could cause issues if the app gets closed before the task gets the chance to finish work - maybe not exactly in the current use cases, but it would probably happen if future users of this toolbox lib put truly long operations in there. Ideally the runner spawn could return a handle the user could join/block on, but that would also require freeing it manually - unless thread semantics were to be applied there and the user could perform a detach operation at their own risk. |
This should not be possible as before starting a concurrent job a busy animation view is put on the view stack which is blocking all input events. In the improbable case in which it happens despite that, no memory leak would occur as the runner will free its resources automatically and the event will be delivered in some other view. Your remark is perfectly valid though, I will make it more clear in the docs that the application is responsible for keeping all relevant contexts valid until the runner completes (the completion is signaled by the Adding a return handle would defeat the purpose of this helper, as it would be no different from using a |
I'm just kind of worried that it might be technically impossible to 100% safeguard against the code getting ripped out from below this thread. It's an edge case, but possibly a valid one nonetheless. Fair warning, it's nitpicky but probably technically valid unless threads keep a strong reference to the application code, which I don't know if they do. My concern is a scenario covered by Raymond Chen on The Old New Thing - while it concerns Windows, the problem is not platform-specific. Consider the following chain of actions:
Now, I am actually not sure how to fix this. On Windows, this class of issues is resolved with One way to solve this would be to let the concurrent runner accept an optional event flag parameter, and have that flag set from inside EDIT: EDIT2: EDIT3: |
@CookiePLMonster |
@DrZlo13 Yes, but the void finish_callback_in_my_app(void* context)
{
// Do stuff
furi_event_flag_set(this_event_will_let_the_app_quit, 1);
// The function epilogue (and RAII destructors in C++) can still execute here, while this code may have been released by the app exiting already
} To fix this, the function signaling the event must either be a noreturn function (which is not viable in this case because teardown after the finish_callback still needs to be executed), or the scheduler must not be allowed to context switch until it's returned from the user callback, hence the idea to employ So, a function like this would be safe from the race, with an additional note in the docs that the finish callback should not perform any blocking tasks (sleep, locking mutexes, waiting on queues). Those shouldn't be performed there even now, because the callback executes on the timer thread, but with the scheduler disabled this becomes a matter of utmost importance. static void concurrent_runner_timer_callback(void* context, uint32_t arg) {
UNUSED(arg);
ConcurrentRunner* instance = context;
furi_thread_join(instance->thread);
if(instance->finished_callback) {
furi_kernel_lock();
instance->finished_callback(instance->context);
furi_kernel_unlock();
}
furi_thread_free(instance->thread);
free(instance);
} EDIT: |
@CookiePLMonster Thanks for a clear example! The solution you have proposed won't work though, because we HAVE to wait on a queue in After some discussion, we have decided that this implementation has lots of potential misuse cases and we'll come up with a better one in the future (since such API would be nice to have anyway). For now, it will be removed from the firmware API and will become a part of the Infrared application, where it serves a very particular purpose and will not suffer from any above mentioned concerns due to a limited scope of usage. P.S maybe I'll get rid of it altogether, because there's a simpler way to do just that with plain threads. |
Could you instead call it at the very end of your work callback and not the finished callback? From the app's point of view this would most likely be equivalent, but then you'd need to implement the finished callback via something like an event if you want to prevent the app from terminating too early. EDIT:
Yeah, since you spin up a new thread every time, you may be right. Maybe it's easier to just spawn a thread with a queue that services those jobs, and avoid any of those issues altogether. |
Looking at the revised approach - what happens if * I realize it is impossible to run two jobs at once with the current design due to these jobs blocking input, but it's a curiosity that popped in my head when I saw this pattern. |
@CookiePLMonster such code will crash if it is compiled with Right now, a separate effort is being made to replace Running into this problem would indeed be improbable in this particular situation. |
What's new
Verification
Infrared -> Universal Remotes -> TVs
Checklist (For Reviewer)