Skip to content

Avoid panics on not owner errors because they should go away#169

Merged
ajroetker merged 2 commits intomasterfrom
avoid-panics-on-not-owner
Jun 29, 2021
Merged

Avoid panics on not owner errors because they should go away#169
ajroetker merged 2 commits intomasterfrom
avoid-panics-on-not-owner

Conversation

@ajroetker
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread mailbox.go
Comment on lines +168 to 170
if err != nil && err != registry.ErrNotOwner {
panic(fmt.Errorf("%w: unable to deregister mailbox: %v, error: %v", errDeregisteredFailed, nsName, err))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Q] How do we know these will go away?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are owned by an expired lease

Comment thread mailbox.go
Comment thread server.go
Co-authored-by: Eric Sniff <epsniff@users.noreply.github.com>
Comment thread mailbox.go
Comment on lines 166 to +171
return err != nil
})
if err != nil {
// Ingnore ErrNotOwner because most likely the previous owner panic'ed or exited badly.
// So we'll ignore the error and let the mailbox creator retry later. We don't want to panic
// in that case because it will take down more mailboxes and make it worse.
if err != nil && err != registry.ErrNotOwner {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should check this earlier, otherwise we're doing unnecessary retries:

	switch {
	case err == nil:
		return false
	// Ignore ErrNotOwner because most likely the previous owner panic'ed or exited badly. 
	// So we'll ignore the error and let the mailbox creator retry later.  We don't want to panic
	// in that case because it will take down more mailboxes and make it worse.
	case errors.Is(err, registry.ErrNotOwner):
		err = nil
		return false
	default:
		return true
	}
})
if err != nil {
	panic(fmt.Errorf("%w: unable to deregister mailbox: %v, error: %v", errDeregisteredFailed, nsName, err))
}

Comment thread server.go
Comment on lines 471 to 478
return err != nil
})
if err != nil {
// Ingnore ErrNotOwner because most likely the previous owner panic'ed or exited badly.
// So we'll ignore the error and let the mailbox creator retry later. We don't want to panic
// in that case because it will take down more mailboxes and make it worse.
if err != nil && err != registry.ErrNotOwner {
panic(fmt.Sprintf("unable to deregister actor: %v, error: %v", nsName, err))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here:

	switch {
	case err == nil:
		return false
	case errors.Is(err, registry.ErrNotOwner):
		err = nil
		return false
	default:
		return true
	}
})
if err != nil {
	panic(fmt.Errorf("%w: unable to deregister mailbox: %v, error: %v", errDeregisteredFailed, nsName, err))
}

@ajroetker ajroetker merged commit c343163 into master Jun 29, 2021
@ajroetker ajroetker deleted the avoid-panics-on-not-owner branch June 29, 2021 23:16
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.

4 participants