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

UndeliverableException in ScanOperationApi21 #609

Merged
merged 4 commits into from Sep 18, 2019
Merged

UndeliverableException in ScanOperationApi21 #609

merged 4 commits into from Sep 18, 2019

Conversation

filippodelfra
Copy link
Contributor

Hi,
I'm submitting this pull request because I'm receiving too many crashes from my application.

Fatal Exception: io.reactivex.exceptions.UndeliverableException: The exception could not be delivered to the consumer because it has already canceled/disposed the flow or the exception has nowhere to go to begin with. Further reading: https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#error-handling | com.polidea.rxandroidble2.exceptions.BleScanException: Scan failed because application registration failed (code 6)
at io.reactivex.plugins.RxJavaPlugins.onError + 367(RxJavaPlugins.java:367)
at io.reactivex.internal.operators.observable.ObservableCreate$CreateEmitter.onError + 73(ObservableCreate.java:73)
at com.polidea.rxandroidble2.internal.operations.ScanOperationApi21$1.onScanFailed + 79(ScanOperationApi21.java:79)
at android.bluetooth.le.BluetoothLeScanner$1.run + 556(BluetoothLeScanner.java:556)
at android.os.Handler.handleCallback + 873(Handler.java:873)
at android.os.Handler.dispatchMessage + 99(Handler.java:99)
at android.os.Looper.loop + 193(Looper.java:193)
at android.app.ActivityThread.main + 6718(ActivityThread.java:6718)
at java.lang.reflect.Method.invoke(Method.java)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run + 493(RuntimeInit.java:493)
at com.android.internal.os.ZygoteInit.main + 858(ZygoteInit.java:858)`

The crash can be reproduced by starting a scan and immediately disposing the observable.

To fix it, I've done a small change in order to have the tryOnError() function inside the ScanOperationApi21 class.

Thanks!

Fix: replaced onError() with tryOnError() in ScanOperationApi21
@CLAassistant
Copy link

CLAassistant commented Sep 13, 2019

CLA assistant check
All committers have signed the CLA.

@dariuszseweryn
Copy link
Owner

Hello,
Thanks for the contribution. Unfortunately until you accept our Contributor Licence Agreement mentioned in the post above I cannot merge your changes.
Thank you!

@filippodelfra
Copy link
Contributor Author

Hi,
Actually I have accepted it multiple times, but here is not updating for some reason.

@dariuszseweryn
Copy link
Owner

Perhaps you have signed it using a different email than account that has made the commit?

@filippodelfra
Copy link
Contributor Author

Yes that was it. Thanks

@dariuszseweryn
Copy link
Owner

Would you mind to create a test suite for this case in OperationScanApi21Test.groovy? It seems to be perfectly testable

@dariuszseweryn
Copy link
Owner

Correct me if I am wrong but the test you have added does pass even before your changes to .tryOnError()?

I was thinking that the test case should add an RxJavaPlugins.setErrorHandler() which would capture all UndeliverableExceptions and it should be queried in then: if it did?

@filippodelfra
Copy link
Contributor Author

filippodelfra commented Sep 16, 2019

You are right.
I've updated the test, let me know if it's working for you now.
Thanks

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

I have looked once more and I think we're almost there. There are three inconsistent things in regards of the test — after that we can merge 👍

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you! 🚀

@dariuszseweryn dariuszseweryn merged commit 492fc54 into dariuszseweryn:master Sep 18, 2019
@dariuszseweryn
Copy link
Owner

dariuszseweryn commented Sep 19, 2019

And it should be available as version 1.10.2 shortly

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.

None yet

4 participants