Skip to content

Early exit when stopListening after shutdown. #705

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.

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

Merged
merged 1 commit into from
Aug 15, 2019

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Aug 14, 2019

No description provided.

@googlebot googlebot added the cla: yes Override cla label Aug 14, 2019
@wu-hui wu-hui requested a review from mikelehen August 14, 2019 19:23
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Hrm... Why does this look so different from https://github.com/firebase/firebase-android-sdk/pull/701/files ?

  1. The test is missing (we generally port tests 1-1 just like we do classes/methods, etc.)
  2. On Android, we didn't need to early-return because the AsyncQueue just silently doesn't run methods after shutdown. So why do we need early-return here?

@mikelehen mikelehen assigned wu-hui and unassigned mikelehen Aug 14, 2019
@mikelehen
Copy link
Contributor

LOL. My comment makes no sense. I thought this was porting to a different platform. I see what happened now.

@mikelehen
Copy link
Contributor

As a sidenote, I think the early return is unnecessary since the AsyncQueue will silently just not run the code after shutdown, but it's also non harmful to make it more explicit that we want to allow this.

@wu-hui
Copy link
Contributor Author

wu-hui commented Aug 15, 2019

As a sidenote, I think the early return is unnecessary since the AsyncQueue will silently just not run the code after shutdown, but it's also non harmful to make it more explicit that we want to allow this.

I want to do this because it makes the intention clear: this method does not call verifyNotShutdown like many others.

@wu-hui wu-hui merged commit 997736d into master Aug 15, 2019
@rlazo rlazo deleted the AllowStopListenAfterShutdown branch September 27, 2019 14:55
@firebase firebase locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants