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

Crashes #598

Merged
merged 2 commits into from
May 4, 2017
Merged

Crashes #598

merged 2 commits into from
May 4, 2017

Conversation

ricardopereira
Copy link
Contributor

No description provided.

@tcard
Copy link
Contributor

tcard commented May 3, 2017

LGTM so far.

@mattheworiordan
Copy link
Member

@ricardopereira @tcard I must admit I am still a little nervous about this fix because as far as I can tell, we don't yet understand how this could have happened. What worries me is that it's plausible we're just covering up a bigger underlying issue, and as far as I can tell, we've not seen any indication that the realtime system is delivering ACK/NACKs out of order for example.

Few things:

  • Have we investigated possible causes for this thoroughly enough? This feels like sticky tape to hide the issue as opposed to fixing the issue.
  • If we add exception reporting, can we send more info when this happens (and other issues like this that we have perhaps recently or some time ago also fixed) to help us diagnose the root cause?

@tcard
Copy link
Contributor

tcard commented May 3, 2017

Have we investigated possible causes for this thoroughly enough?

No. We're focusing first on handling unexpected conditions gracefully.

If we add exception reporting, can we send more info when this happens (and other issues like this that we have perhaps recently or some time ago also fixed) to help us diagnose the root cause?

That's a good idea, I'll add support for sending Sentry reports without crashing.

@ricardopereira
Copy link
Contributor Author

ricardopereira commented May 3, 2017

I was afraid that the incoming messages were processed with a different order but I did some tests and the dispatch queue is well implemented. The queue is serial so each task is executed one at a time in the order in which they were added to the queue.

@mattheworiordan
Copy link
Member

The queue is serial so each task is executed one at a time in the order in which they were added to the queue.

Ok, well apparently not if it's crashing 😏

On a serious note though, no other library has reported this problem so it seems unlikely this is a realtime issue. As such, the iOS lib is probably doing something odd in this area.

@ricardopereira
Copy link
Contributor Author

On a serious note though, no other library has reported this problem so it seems unlikely this is a realtime issue. As such, the iOS lib is probably doing something odd in this area.

Yes, I agree but I don't know where to look. I was suspicious about the incoming messages order but that's something that is working as expected.

@tcard Do you have any suggestion?

@tcard tcard merged commit e8408ae into master May 4, 2017
@tcard tcard deleted the failures-more-context branch May 4, 2017 17:42
tcard pushed a commit that referenced this pull request May 15, 2017
* Ack: fail gracefully when realtime receives a serial greater than expected

* Transport: should guarantee the incoming message order
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.

3 participants