search

rusel1989 / react-native-bluetooth-serial

Port of https://github.com/don/BluetoothSerial for react native
476 stars 293 forks source link

Proper resources disposal on "reload" #15

Open wmaca opened 7 years ago

wmaca commented 7 years ago

When I press "Reload" or the application reloads due to Live Reload being enabled, the onDestroy method of the module is not called. Because of that, the sockets (and streams) are no correctly disposed.

When the reload is finished, I can no longer open a bluetooth socket. It requires me to disable and enable bluetooth, or to restart the application.

Is there ant callback or method I could implement that would correctly dispose these sockets when I reload my application?

rusel1989 commented 7 years ago

@waltermacambira hey, yep this is bothering me too, there should definately be some way how to do it, I will look into it

wmaca commented 7 years ago

@rusel1989 I am not familiar with Android development, but if you need any help on creating a PR for this, please let me know. I could not find in 'react-native' docs anywhere saying where to add a callback to make this kind of clean up. One way around it, would be to detect the socket is in this state, cleaning it if so.

wmaca commented 7 years ago

@rusel1989 Solved at least for Android. I made the service variable static, so I could properly release the resources (service.stop()) when creating a new one.

rusel1989 commented 7 years ago

@waltermacambira Cool, so can you please submit pull request with your fixes ?

wmaca commented 7 years ago

@rusel1989 even if it is android only?

timscott commented 7 years ago

@waltermacambira - I would appreciate an android only fix. Thanks.

timscott commented 7 years ago

@waltermacambira - Is the fix you mean change this line to:

private static RCTBluetoothSerialService mBluetoothService;

I need to get this working, and I will make my own fork if necessary.

By the way, this is not just a development headache. CodePush deployments corrupt the connection. Any user that is connected at the time a CodePush update is applied will never be able to connect again until he manually unpairs via device Settings.

Also by the way, I attempted to implement a workaround using Bluetooth.unpairDevice. However, it seems that function is missing for some reason. I'm not sure why, because it seems to be implemented here. Anyone have a clue about that?

timscott commented 7 years ago

I tried changing this line to:

private static RCTBluetoothSerialService mBluetoothService;

Did not fix the problem.

By the way, looking at output from adb logcat I never see the message logged at this line. In fact I never see any of the messages that begin with "Host ". Could it be the problem is that for some reason these lifecycle methods are never being called?

CORRECTION: This does fix the problem. Yay. (I made the dumb error of forgetting that native updates require a new APK because they are not applied by CodePush.)

wmaca commented 7 years ago

Yes, the fix can be as simple as that. A better disposal of the streams would be nice too, but I did not see it as necessary.

timscott commented 7 years ago

Alas, this fix is not working for me. While it appears to "fix" the connect error (I can successfully connect) the connection is not functional.

In my application I have implemented a request-response pattern. I write an AT command, then recursively dead until I reach either "OK" or "ERROR" or timeout. After the re-connect, the reads never return anything.

The problem is in fact worse because to achieve a good state I need not only to un-pair the device but to also restart my application.

timscott commented 7 years ago

My problem seems to be solved by upgrading to v1.0.0-rc1.

I neglected to mention before that I have been using v0.1.6, which was the current NPM version when I started using this module, and it's still the most recent tag. That's the version I applied @waltermacambira's suggested change to.

Yesterday I tried v1.0.0-rc1, which I guess it was recently made the current version on NPM, but it completely failed. Today I diagnosed the problem as due to a breaking change: the read function was renamed readFromDevice. Since neither function was ever mentioned in the documentation of this module, it took a lot of digging to diagnose the problem.

I plan to make a PR with two things:

  1. Add a function named read in index.js. This will bring semantic consistency with the write function, thus reducing surprise.
  2. Add documentation for read and other undocumented functions to the README, which will potentially save people lots of time using this module.