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.

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

fix: invoke result.success after pushReset #479

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ttypic
Copy link
Contributor

@ttypic ttypic commented Sep 15, 2023

To avoid timeouts invoke result.success after pushReset

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

Nice fix 👍
Timeout errors were misleading

@@ -648,6 +648,7 @@ private void pushReset(@NonNull MethodCall call, @NonNull MethodChannel.Result r
instanceStore.getPush(ablyMessage.handle)
.getActivationContext()
.reset();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main problem with reset is that it clears ably client from activationContext and it breaks everything until app is reloaded. There are 2 possible solutions:

  • Reinstall ably client in flutter (I've checked locally and it works)
instanceStore.getPush(ablyMessage.handle)
              .getActivationContext().setAbly((AblyRest) instanceStore.getAblyClient(ablyMessage.handle));
  • Do not clear ably client in reset (it looks for me that it is unnecessary, because ably client doesn't store any state) PR here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on my observations while reading the code, I understood that reset breaks activation/deactivation state machine. It was never been a proper solution to the problem. As per PR (#357), it was introduced as an experimental feature that was never tested as such and I couldn't see any test cases available for the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sacOO7 actually reset works in one particular very specific scenario: when ably environment has changed. If this happened activate/deactivate stop working. I agree that it's not very good solution, but it looks like it works. Basically it doesn't break state machine it simple delete it and recreate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants