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

did remove crashing asserts #43

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

Conversation

alekseykolchanov
Copy link

In production we had too many crashes in sendPing function. Crashlytics shows crashes on 'assert()' put in sendPing function

There are two issues, I believe, related to those asserts: #36
#34

@melon8
Copy link

melon8 commented Feb 26, 2018

I think you don't need return in line 503. Because if bytesSent < 0, it will go to call the delegate method "ping:didFailWithError:" in line 615. The delegate method will output more information.

@lawmaestro
Copy link

Just run into this issue! Is there any update on the status of this PR? Thanks.

@alekseykolchanov
Copy link
Author

@melon8 Thank you for your suggestion.
There is a 'switch' on line 493. In the case statements author defines what type of packet to send and invokes the code which sends the packet and initiates local variable 'packet' (defined on line 487)
Since author uses that 'packet' variable later in the method and initially put assert for 'default' case on line 500, I assume it is safer to just return from the method

@alekseykolchanov
Copy link
Author

@lawmaestro Hello!
There were no updates to the PR

@lawmaestro
Copy link

I meant is the PR likely to get merged shortly? Just it addresses an issue I’m running into. Thanks.

@Roman-Scherbakov
Copy link

@alekseykolchanov is this pull request fix crashes? Because it is reproduced in very rare cases. And are you sure that this change fix asserts: #36 #34?

@robinschmied
Copy link

We can't reproduce the issue, but we are getting crash reports from those 2 lines. We believe that the changes would fix the issue.

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.

5 participants