Skip to content

dbus: Don't deadlock on shutdown#514

Open
purpleidea wants to merge 1 commit intocoreos:mainfrom
purpleidea:bug/no-deadlock
Open

dbus: Don't deadlock on shutdown#514
purpleidea wants to merge 1 commit intocoreos:mainfrom
purpleidea:bug/no-deadlock

Conversation

@purpleidea
Copy link
Contributor

When cancelling the ctx, if we have a pending channel send, then we'll deadlock. The correct pattern is to abort if ctx exits.

We also close the channels when the sender closes.

@purpleidea
Copy link
Contributor Author

Not sure how I didn't notice this deadlock sooner. I think it's important to get this in somehow.

Copy link
Collaborator

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks, just one minor thing. Also CI just get fixed by #515

statusChan <- changed
select {
case statusChan <- changed:
case <-ctx.Done():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should return here.
Otherwise if the timer channel below is completed already we might end up doing another loop iteration for no reason.

errChan <- err
select {
case errChan <- err:
case <-ctx.Done():
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

When cancelling the ctx, if we have a pending channel send, then we'll
deadlock. The correct pattern is to abort if ctx exits.

We also close the channels when the sender closes.
@purpleidea
Copy link
Contributor Author

Thanks, just one minor thing

Not a minor thing, I just plainly was coding fast and not paying attention. All fixed and rebased. Cheers

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