RxJava2 #67
RxJava2 #67
Conversation
|
|
JamesMcIntosh
force-pushed
the
RxJava2
branch
from
f6d5534
to
c71d4f4
Compare
|
@dariuszseweryn @mikolak Any reasons not to pull this in? This will resolve the leak issue that is present in the current version of the Adapter (due to the old version of rxandroidble library that is used). |
|
Hi, @JamesMcIntosh! Which library have you checked it against, FlutterBleLib or react-native-ble-plx? Have you encountered any performance issues? |
|
Hi @mikolak , I've tested it against FlutterBleLib. I haven't encountered any performance issues, it's been pretty smooth. I would expect the only time it should be of any real significance is if you are monitoring a characteristic which is pushing a lot of data and the device can't keep up and buffers till out of memory. I guess you could pass in an explicit BackpressureStrategy enum to specify the strategy which it employs. |
|
I didn't remember correctly - there are changes to how errors work. If two functions in a chain throw error, then only the first error is delivered to the error handler and the other one goes to a globally registered error handler and there can be only one such registered, so if there were other native dependencies setting that there would be a race condition on which one is used. Since we don't want to use the global listener, there supposedly is a risk of a crash killing the application. The possible culprit is setting notifications. I'm not sure how - possibly when both the notification stream and writing the descriptor result in an error. I'll try to get back to you once I have something more solid. Can you check that scenario? I might release them as a dev version. |
|
Hi @mikolak, The documentation suggest that intermediate libraries should not be setting a global error handler, instead they suggest that the final application is the one to set this handler. To stick with this recommendation then I would recommend updating the documentation in FlutterBleLib and react-native-ble-plx to add their setup instructions details of the setting a global error handler. I would also recommend a major version bump to these libraries even though it is dependency upgrade should normally be a minor version. Additionally there could be a check added to the libraries which looks to see if the global error handler has actually be registered and log a warning message / exception. |
|
Hello @JamesMcIntosh Thanks for your input. The problem with using RxJava2 with a global error handler is that this may interfere with other libraries that user RxJava2 under the hood making it a bad experience and potentially long hours spent on looking for hard to trace crashes. I have a plan, for quite a while now, to create a RxJava2 based API that would behave differently — it would complete all streams originated from I dream of a set of libs that have stable, up to date dependencies and are as easy to use as possible. I want to get to that but having experience with RxAndroidBle I did not decided to bump its version in this project because of potential fallout. |
|
Hi @dariuszseweryn, Thanks for the background information, it sounds like a solid refactor to rebuild the API, encapsulating the RxJava2 implementation should shield the user from any problems. I feel like I'm not seeing the problem with RxJava2 the same way as you as I'm not sure how it may interfere with other libraries, is there something specific you're thinking of? The main issue I had was the RxAndroidBle - RxJava1 dependency in this library is that it doesn't work with the bluetooth device I'm connecting to and has fallen behind with fixes. I've used RxAndroidBle - RxJava2 for 2 months now and it's worked perfectly, in my testing I also see that RxJava2 will stop development Feb 2021, is there a plan to migrate up to RxJava3 also? |
|
@mikolak @dariuszseweryn guys, so what's your take on this? Any chances on pulling in that change soon? Or have you got some other proposal maybe (with a timeframe, if possible)? Those leaking receivers is a real issue that would be nice to address you know :) |
|
I'll be releasing this as 1.0.0-beta. Until it's battle-tested I'll be keeping it so. |
|
@mikolak Will there be a beta version of the Flutter BLE plugin too that includes this beta version of the adapter? Or are you planning to verify it using other plugins? |
|
Yeah, preparing betas of MBA and FBL. I was planning to make you the tester. 😉 |
|
Sounds good! @mikolak by the way, is BleEmulator affected by this change (I guess it is?) If so, would it be possible to post a beta for it too (we're using it as well)? |
|
@mikolak you should consider bumping version number of the RxAndroidBle lib once again now that @dariuszseweryn have merged another leak-related fix.. dariuszseweryn/RxAndroidBle#708 😀 |
It's not, BLEmulator only uses the BleAdapter interface. Should work just as well with this new version, unless pub complains about versions. Let me know if that happens.
One step at a time. 😉 If all works well, I'll get to updating it, though not sure if that will happen in December. |
Hi @JamesMcIntosh, Thanks, |
|
Hi @b055man, Yes, I set call the The There is a good explanation and example in the RxJava documentation. @b055man would you be able to share what type of @mikolak @dariuszseweryn There is some discussion in the RxJava documentation above about catching Kind regards |
|
@JamesMcIntosh ah, thank you. I've missed the documentation update apparently.. :) It's a I wonder a bit why that |
|
@b055man Are you able to determine what the other Exception was? As for seeing two state changes, I had a look at the Plugin code and you may end up with 2 messages if you call createClient without having destroyed it between.I think this is a bug in |
|
@JamesMcIntosh I think the other one was I'm calling the 'createClient' method only once, upon the app startup and clean up the client upon the app exit.. Not sure how that could end up being called twice, but thanks for catching that, this sounds like a thing to check definitely! |
|
@b055man Have a look in the Android debug logs for |
|
Hot restart, perhaps? It just drops any Dart state, so it could potentially create second client without destroying the previous one. |
|
@mikolak Does it make sense to guard against the creation on multiple clients then in |
…2_test * commit '7b4c11dd2d94bfd492dbc82bad6e0fde0112e3ae': Release 1.0.0-beta RxJava2 (dotintent#67)
Upgrades the RxAndroidBLE library to the latest version and basic RxJava2 changes to support the new version.
I haven't looked at changing the architecture of the project when implementing the RxJava2 changes or updating the java coding style.