Try to prevent command lockup #555
Merged
+34
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an attempt to fix a lockup issue that we see in the Swift example app when the camera capture status subscription is used. Presumably, the lockup has nothing to do with the actual camera capture status but it just leads to the right timing to trigger something else. What happens is that `receive_timeout()` is called but then the command queue turns out to be empty (returns `nullptr`). After that, no more commands can be processed and we keep getting the warning: "Command ack not matching our current command: 257". This makes sense because we stop doing work, so send any commands as long as `_state` is `State::WAITING` and there is no other timeout that would reset this. Therefore, this change should resolve that case by reseting `_state`, so that we can keep going. However, this change does not fix or explain why the queue is empty in the first place and out of sync with `_state`.
This is an attempt to fix the issue described in the previous commit message. We should remove any timeout handlers before poping work from the queue to prevent that `receive_timeout()` is called in the moment when the queue is already empty and doesn't know what to do about it. Basically, we always try to follow this order to prevent things out of sync: 1. Set the `_state` 2. Unregister timeout handler 3. Pop from queue. 4. Call user callbacks. It is still unclear to me how the lockup case exactly happens. I can't see how `_state` can ever be out of sync with the queue given the protecting mutexes.
This has been addressed in the meantime.
This is supposed to be just a bugfix.
|
I tested this and it doesn't break the integration test, so I suggest it's worth a try in the iOS example. |
julianoes
changed the title
JonasVautherin
approved these changes
Sep 22, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt to fix a lockup issue that we see in the Swift
example app when the camera capture status subscription is used.
Presumably, the lockup has nothing to do with the actual camera capture
status but it just leads to the right timing to trigger something else.
What happens is that
receive_timeout()is called but then the commandqueue turns out to be empty (returns
nullptr). After that, no morecommands can be processed and we keep getting the warning:
"Command ack not matching our current command: 257".
This makes sense because we stop doing work, so send any commands as
long as
_stateisState::WAITINGand there is no other timeout thatwould reset this.
Therefore, this change should resolve that case by reseting
_state, sothat we can keep going.
However, this change does not fix or explain why the queue is empty in
the first place and out of sync with
_state.