Fix ConcurrentModification Exception on asWritableMap on GATT connected peripherals. #1264
Conversation
example/package-lock.json
Outdated
There was a problem hiding this comment.
Sorry for a drive by comment, but why are you deleting the lock file for the example?
| @@ -220,13 +221,12 @@ public WritableMap asWritableMap(BluetoothGatt gatt) { | |||
| WritableArray characteristicsArray = Arguments.createArray(); | |||
|
|
|||
| if (connected && gatt != null) { | |||
| for (Iterator<BluetoothGattService> it = gatt.getServices().iterator(); it.hasNext();) { | |||
| for (Iterator<BluetoothGattService> it = new CopyOnWriteArrayList<>(gatt.getServices()).iterator(); it.hasNext();) { | |||
There was a problem hiding this comment.
I'm doubtful that this will actually fix the problem, but merely hide it.
There was a problem hiding this comment.
At least will solve for iterator .next() presenting sudden crash, not hiding it Services and caracteristics are being changed by the time asWritable method is called. I dont see any problem with that since asWritable role is just parsing as a rn bridge object for events and callbacks. Also copy on write will ensure last known characteristics and services for that.
There was a problem hiding this comment.
Would you suggest a better approach on that?
There was a problem hiding this comment.
I would really be interested in the actual reason why the ConcurrentModificationException is thrown.
Since we (i.e. this code) isn't modifying the iterator (unless we call the next call), it must mean that the returned ArrayList is not a fresh copy, but a reference that gets modified by someone else while we're iterating on it.
I tried (briefly) to find the source code for BluetoothGatt but could only manage to find a source that was incorrect (based on the file length and the reported line number on the stack trace in #1229).
If figuring that out is out of scope for this fix (which is completely understandable), I would rather just take a complete copy of the returned array (instead of trying to be fancy with CopyOnWriteArrayList).
There was a problem hiding this comment.
I would really be interested in the actual reason why the
ConcurrentModificationExceptionis thrown. Since we (i.e. this code) isn't modifying the iterator (unless we call thenextcall), it must mean that the returnedArrayListis not a fresh copy, but a reference that gets modified by someone else while we're iterating on it.I tried (briefly) to find the source code for
BluetoothGattbut could only manage to find a source that was incorrect (based on the file length and the reported line number on the stack trace in #1229).If figuring that out is out of scope for this fix (which is completely understandable), I would rather just take a complete copy of the returned array (instead of trying to be fancy with
CopyOnWriteArrayList).
My guess is:
1) Somehow the number of services and characteristics can change during runtime and alongside of asWritableMap is being called.(hard to know probably not)
2) The order of callbacks on js bridge can make this happen. Something like discoverServices and getConnected/getDisconnected(or some other callbacks that use peripherals) being called at the same time.
So this quickfix makes the method concurrent proof.
Still i'll try to reproduce that on my Android 14 when i have spare time with the available api to trying reproducing it.
About the implementation: i agree, do you wanna make that change or i can just abort this pull request and create another one with straight copy of services and characteristics list?
There was a problem hiding this comment.
I don't have the bandwidth to work on this (nor do I have any permissions to merge anything).
Fixes Concurrency problems on parsing back Peripherals objects.
Aims to fix the occurrence of problem as reported on issue #1229