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.

Sign up for GitHub

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

Jump to bottom

Ensure ExternalInputDeviceHub::remove_observer() synchronizes memory across threads #360

Merged
merged 3 commits into from May 11, 2018
Merged

Ensure ExternalInputDeviceHub::remove_observer() synchronizes memory across threads #360

merged 3 commits into from May 11, 2018

Conversation

AlanGriffiths
Copy link
Contributor

@AlanGriffiths AlanGriffiths commented May 9, 2018 •
edited

Ensure ExternalInputDeviceHub::remove_observer() synchronizes memory across threads. (Fixes #359)

RAOF
RAOF approved these changes May 10, 2018
View reviewed changes
Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a sensible case of making the function name match its behaviour!

bors r+

bors bot added a commit that referenced this pull request May 10, 2018
@bors @AlanGriffiths
Merge #360
0bb12e2 
360: Ensure actions queued by GLibMainLoop::enqueue_with_guaranteed_execution() are executed r=RAOF a=AlanGriffiths

Ensure actions queued by GLibMainLoop::enqueue_with_guaranteed_execution() are executed. (Fixes #359)

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
@bors
Copy link
Contributor

bors bot commented May 10, 2018

Build failed

@RAOF
Copy link
Contributor

RAOF commented May 10, 2018 •
edited

Hm, actually, on closer inspection of the surrounding code…

I'm not sure that this is the right solution; particularly, it changes the behaviour so that all idle callbacks are always run, and in an unusual context - the GSource destructor will be run during GMainLoop destruction, rather than at ::stop() time, I believe.

That said, I don't see what's wrong with the current enqueue_with_guaranteed_execution() implementation (other than transition running = false → running = true being insufficiently locked, but that doesn't seem to explain your symptoms)

bors r-

@AlanGriffiths
Copy link
Contributor Author

On closer examination of the logic I agree. Here's a correct solution.

@AlanGriffiths AlanGriffiths changed the title Ensure actions queued by GLibMainLoop::enqueue_with_guaranteed_execution() are executed Ensure ExternalInputDeviceHub::remove_observer() synchronizes memory across threads May 10, 2018
@AlanGriffiths
Copy link
Contributor Author

Although... in the case I'm debugging I thought the update to "removed" ought to be on the same thread.

But I have failed to reproduce this failure mode with this change, so maybe the removal hangs before we close the main loop? But surely that would happen more often?

@AlanGriffiths AlanGriffiths mentioned this pull request May 10, 2018
Closed
gerboland
gerboland approved these changes May 11, 2018
View reviewed changes
Copy link
Contributor

@gerboland gerboland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not been able to get the hang with this either. Code change makes sense to me
bors r+

bors bot added a commit that referenced this pull request May 11, 2018
@bors @AlanGriffiths
Merge #360
1d3c7a7 
360: Ensure ExternalInputDeviceHub::remove_observer() synchronizes memory across threads r=gerboland a=AlanGriffiths

Ensure ExternalInputDeviceHub::remove_observer() synchronizes memory across threads. (Fixes #359)

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
@bors
Copy link
Contributor

bors bot commented May 11, 2018

Build succeeded

@bors bors bot merged commit 860d8e6 into master May 11, 2018
@bors bors bot deleted the fix-359 branch May 11, 2018 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerboland gerboland gerboland approved these changes

@RAOF RAOF RAOF approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AlanGriffiths @RAOF @gerboland