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

iteration / thread race condition fixes #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

iteration / thread race condition fixes #33

wants to merge 1 commit into from

Conversation

mweal-ed
Copy link
Contributor

@mweal-ed mweal-ed commented Jul 17, 2017

This is a fix for issue 31 where there is an occasional race condition because the glib GMainContext g_gattlib_thread->loop_context is run in two threads (explicitly not allowed in glib documentation).

I have provided 2 different fixes controlled by defining USE_THREAD in gattlib_internal.h. The default operation does gattlib_iteration's as required. When USE_THREAD is defined a main loop is run in a separate thread (this is the way it is done in the existing code) for processing glib events. gattlib_iteration is still called but g_main_context_iteration is only called if gattlib_iteration was called from the same thread as the g_main_loop.

I do not like the USE_THREAD mode because:

  1. I do not think it is necessary
  2. The way it is implemented it is not very cpu friendly because it will be continuously looping on the event complete variables.
  3. I have tried using a mutex or semaphore for watching the thread complete variables instead, but they seem to create the same race conditions we started with. This makes me think there is something in the Gatt code that is not thread safe. I have not been able to track it down.

@mweal-ed
Copy link
Contributor Author

mweal-ed commented Jul 17, 2017

Note: I was probably a little pre-mature on submitting this. If it is used without "USE_THREAD", it will break the _async functions.

@oliviermartin
Copy link
Contributor

Thanks a lot @mweal-ed to have found the reason of this bug. My understanding is without USE_THREAD (default) async functions do not work anymore and with USE_THREAD it fixes the issue. Should we enable USE_THREAD by default?
Should I wait before merging it with USE_THREAD? I will come back on gattlib in a couple of weeks. I will have more time to test it and/or offer an optimal solutions.

@mweal-ed
Copy link
Contributor Author

You are correct, however I think there may be some hidden problems when using threads.

This was referenced Jul 18, 2017
@mweal-ed
Copy link
Contributor Author

mweal-ed commented Aug 8, 2017

I am not that experienced with git, so let me know what you want me to do to so we can proceed with these (and my other) fixes.

@mweal-ed mweal-ed closed this Aug 8, 2017
@mweal-ed mweal-ed reopened this Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants