Skip to content
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

How to safely call hook_stop not from a callback? #85

Open
anthonybilinski opened this issue Jul 9, 2020 · 6 comments
Open

How to safely call hook_stop not from a callback? #85

anthonybilinski opened this issue Jul 9, 2020 · 6 comments
Assignees
Labels

Comments

@anthonybilinski
Copy link
Contributor

Since hook_run blocks the thread until stop is called, the only places hook_stop can be called on the same thread are in callbacks, which is what the two demos do. If an application wants to to stop hook on demand though, relying on user input to cause a callback while trying to shutdown seems unreliable.

Alternatively calling hook_stop from an outside thread looks unsafe since hooks API are not thread safe. This might not be that bad in practice since hook_run looks like it will be blocked on other library calls and not accessing its structure data after startup, but this still doesn't give any guarantees of thread safety.

A possible workaround might be to stop from the callback and inject an event from the application to force a callback, but this seems pretty convoluted.

I think the API would be easier to work with if hook_run was called periodically from the application and returned, instead of being called once and blocking, allowing the application to call other API in between checking for new keys.

@kwhat
Copy link
Owner

kwhat commented Jul 10, 2020

So the library leaves the majority of async work up to the implementing developer, so how it gets handled is going to be up to your implementation. Calling hook_stop directly from the callback should be fine as it just unregisteres the hook and shouldn't have any thread safety issues. With that said, starting the hook back up from the same context is going to cause issues.

There are some technical reasons things are setup the way they are. In order to consume events and stop propagation, the callback for events needs to happen on the OS dispatch loop and block until the library is done with it. This is why this library causes all input to freeze when the debugger hits a breakpoint. All UI applications are already going to have a dedicated thread of UI events and the intended design of the library was to allow the implementing developer to choose when to copy the events from the OS dispatch queue and deliver them via that UI thread. The stop/start hook function was really intended to be called from this UI theads context where there should only be thread safety concerns in the hook callback and any other threads your program created.

I had originally created an additional dispatch thread specifically for this library, but I abandoned the idea as it added even more complexity to the problem and did little for reliability. If you do not need to prevent event propagation, I would recommend that you copy the data you need on callback and deliver the info on another thread. As long as start/stop are called from this other thread, you should only need to make sure that after the callback returns after stop is called, before you call start again. That should be easy enough with a semaphore and wrapping function.

@anthonybilinski
Copy link
Contributor Author

Calling hook_stop directly from the callback should be fine as it just unregisteres the hook and shouldn't have any thread safety issues.

My issue with this approach is that then e.g. shutting down the app relies on the user creating events to call the callback. If the user isn't moving their mouse or hitting keys, the callback won't be called and the app won't be able to call hook_stop from the callback. For this reason I want to call hook_stop from outside of any of the callbacks.

I would recommend that you copy the data you need on callback and deliver the info on another thread. As long as start/stop are called from this other thread, you should only need to make sure that after the callback returns after stop is called, before you call start again.

I'm not sure I understand what you're describing here. If I have thread A, e.g. a UI/application thread, and thread B, e.g. hook's thread, my issue is that:

  • Thread B must be the caller of hook_run since it blocks.
  • Thread B is active in callbacks, and could safely call hook_stop there, but due to it them being dependent on user input it seems non-deterministic
  • Thread A wants to shut down the program, but can't safely call hook_stop.

I think you're describing:

  • Thread B call hook_run.
  • Thread B calls hook_stop from callback.
  • Thread B doesn't call hook_run again until all callbacks are exited.

Which is fine with me, but I still have issues notifying thread B at all that it should thread_stop without relying on user-activated callbacks.

@kwhat
Copy link
Owner

kwhat commented Jul 10, 2020

My issue with this approach is that then e.g. shutting down the app relies on the user creating events to call the callback.

Your going to need some event somewhere to decide when to call the function ;) Either the user event though the hook callback, an event timer, or via the UI for the program.

Let me setup some threads:

  • Thread A: Lets call this the int main() primary thread
  • Thread B: This can be the hook_run() thread
  • Thread C: UI Event Dispatch Thread or Event Timer Thread

Thread B must be the caller of hook_run since it blocks.

Sorry, its been a while and I forgot to account for that. Yes, because hook_run blocks that would need its own thread as described.

Thread B is active in callbacks, and could safely call hook_stop there, but due to it them being dependent on user input it seems non-deterministic.

Yes, Thread B (run_hook) is the same thread you are in with the hook callback used by the library. This doesn't need to be the same thread calling hook_stop, but it can be. I originally assume hook_stop would more likely be called on a different thread like Thread C or Thread A. Because Thread B is part of your program and the library doesn't have an event thread, there shouldn't be any thread safety surprises that were abstracted away by the library.

Thread A wants to shut down the program, but can't safely call hook_stop

So I think the confusion is that hook_stop can be called from Thread C or Thread A without issue. You really don't even need to do any thread safety to call stop from these threads unless your specific implementation requires it. Thread B will stop when you call hook_stop as hook_run will return and the thread should exit naturally. Because of this, you would probably need to create a new thread when you restart the hook. This is where that wrapping function for hook_start/stop that I was talking about would be useful for re-creating that thread (Thread B) and handling thread safety you may need.

@kwhat
Copy link
Owner

kwhat commented Jul 10, 2020

Little update, it just went though the code for Windows and Mac and it looks like I am not trying to stop hook_run and just signaling the calling thread to stop. That's why there isn't a safety issue on stop. Not sure if its the best approach but I am also not sure how to make it any better.

@anthonybilinski
Copy link
Contributor Author

Your going to need some event somewhere to decide when to call the function ;) Either the user event though the hook callback, an event timer, or via the UI for the program.

True, by "event" there I meant an event that forces the hook callback. I'm thinking that maybe:

  1. User clicks X and lets go of keyboard and mouse.
  2. Hook callback gets called, but it doesn't yet know that the app wants to exit so doesn't call hook_stop.
  3. App gets notification, starts shutting down and passes state down to where hook's callback can check it.
  4. Hook's callback doesn't get called again, and the app hangs.

If I'm missing something here and there's a way to cause or guarantee that dispatch_proc will be called on an ongoing basis regardless of user action, then stopping from the callback will be fine.

This doesn't need to be the same thread calling hook_stop, but it can be.

Ah ok this would completely solve the issue for me. Looking at the implementation though, there's non-atomic non-mutext-protected state hook shared by hook_run and hook_stop in the x11 implementation, meaning that there could theoretically be very delayed or no data propagation between the threads (I think dependent on cache space being needed by another thread and the updates being written to main memory) or uninitialized data accessed in hook_stop, since hook_run could still be initializing when hook_stop is called. For the Windows implementation it looks like just hook_thread_id is shared, and for Darwin's implementation event_loop and event_loop are shared.

It might be mostly theoretical that hook_stop would be called by another thread before hook_start is done initialization, or that the data wouldn't be pushed from hook_run's thread to main memory by the time hook_stop is called, but it doesn't seem like there's a guarantee.

I think this could be resolved by mutex protecting the shared state for each implementation and locking that in both hook_run and hook_stop when accessing the state, which should make calling hook_stop not from a callback guaranteed safe.

@kwhat
Copy link
Owner

kwhat commented Jul 10, 2020

I believe you are correct on the mutex, should be addressed. 👍

@kwhat kwhat self-assigned this Sep 28, 2021
@kwhat kwhat added the bug label Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants