Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Mar 9, 2020

Since Mjolnir is a shared instance, we need to be a bit more careful about
clearing internal state than we do for components. This clears watcher and
dispatcher refs in case stop is called multiple times (which can happen in
prod as well as tests).

Since Mjolnir is a shared instance, we need to be a bit more careful about clearing internal state than we do for components. This clears watcher and dispatcher refs in case `stop` is called multiple times (which can happen in prod as well as tests).
@jryans jryans requested a review from a team March 9, 2020 15:26
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

seems sensible, assuming you can get the tests to pass again.

Comment on lines -74 to -75
// Only the tests cause problems with this particular block of code. We should
// never be here in production.
Copy link
Member

Choose a reason for hiding this comment

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

These two lines are the reason your tests are failing, I'll bet.

@jryans
Copy link
Collaborator Author

jryans commented Mar 9, 2020

Looks like no change was needed, just intermittent tests being flaky.

@jryans jryans requested a review from turt2live March 9, 2020 16:12
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I guess so, though I'm not very confident the try/catch can be safely removed.

@jryans
Copy link
Collaborator Author

jryans commented Mar 9, 2020

Hmm, well, your commit message from back was suspecting that stop is called multiple times, and that's what this change should help with. Let's watch for any issues and see how it goes.

@jryans jryans merged commit d2cd64d into develop Mar 9, 2020
@turt2live
Copy link
Member

oh, then this is fine. I thought the try catch was for something else...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants