GBPing icon indicating copy to clipboard operation
GBPing copied to clipboard

did remove crashing asserts

Open alekseykolchanov opened this issue 7 years ago • 7 comments

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: https://github.com/lmirosevic/GBPing/issues/36 https://github.com/lmirosevic/GBPing/issues/34

alekseykolchanov avatar Dec 25 '17 06:12 alekseykolchanov

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.

melon8 avatar Feb 26 '18 08:02 melon8

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

lawmaestro avatar Mar 28 '18 15:03 lawmaestro

@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 avatar Apr 02 '18 06:04 alekseykolchanov

@lawmaestro Hello! There were no updates to the PR

alekseykolchanov avatar Apr 02 '18 06:04 alekseykolchanov

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

lawmaestro avatar Apr 02 '18 09:04 lawmaestro

@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?

Roman-Scherbakov avatar Jun 11 '18 09:06 Roman-Scherbakov

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.

robinschmied avatar Jun 29 '18 12:06 robinschmied