Fix/workaround a crash when ReactInstance is disposed while AsyncStorage still doing work #1888
Conversation
…sposed on another thread while the AsyncStorage mutex is held. Thanks to my colleagues for finding this and providing a fix.
| [Test] | ||
| public void AsyncStorageModule_DoesNotInvokeCallbackAfterDispose() | ||
| { | ||
| callback = new MockCallback(invoke => throw new Exception("should not be called")); |
There was a problem hiding this comment.
To make this test a bit more deterministic, can we use a ManualResetEvent and call Wait with a timeout (say 500ms)? Otherwise, if this regresses, the exception that gets thrown could become a red herring / cause another test to fail / cause the test runner to crash.
| { | ||
| try | ||
| { | ||
| var token = _cancelSource.Token; |
There was a problem hiding this comment.
Can we use the mutex and check the IsCanceled property rather than relying on exception behavior? I know we'll likely need the catch blocks anyway, but it might prevent a few frequent exceptions.
| await _mutex.WaitAsync(token).ConfigureAwait(false); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { |
There was a problem hiding this comment.
can we add a when clause to this so we only catch when the OperationCanceledException is sourced from the correct cancellation token?
| @@ -274,6 +289,8 @@ public override string Name | |||
|
|
|||
| public override void OnReactInstanceDispose() | |||
| { | |||
| _cancelSource.Cancel(); | |||
There was a problem hiding this comment.
If you use CancellationDisposable from System.Reactive, it only requires a Dispose call, and I think it will protect you when accessing the token (so you won't need the ObjectDisposedException catch block below).
| @@ -274,6 +289,8 @@ public async void getAllKeys(ICallback callback) | |||
|
|
|||
| public override void OnReactInstanceDispose() | |||
| { | |||
| _cancelSource.Cancel(); | |||
There was a problem hiding this comment.
Should we be taking the mutex here too?
What happens if a read/write is in progress where the mutex was taken.
We dispose the mutex and then the read/write completes and tries to release the mutex (which has now been disposed)
Another alternative I can think of is to replace all existing calls to _mutex.Release with calls to a new function SafeReleaseMutex which check IsCancelledOrDisposed before releasing the mutex.
| @@ -225,6 +236,8 @@ public async void multiMerge(string[][] keyValueArray, ICallback callback) | |||
| [ReactMethod] | |||
| public async void clear(ICallback callback) | |||
| { | |||
| if (await IsCancelledOrDisposed()) return; | |||
|
|
|||
| await _mutex.WaitAsync().ConfigureAwait(false); | |||
There was a problem hiding this comment.
What happens to these WaitAsync calls which are already in progress? We should pass the cancellation token to them to ensure that they are cancelled before we dispose the mutex.
I tried prototyping a race condition and looks like not cancelling the wait results in a the thread waiting there for ever.
|
@matthargett I was looking at a similar issue we have in our code, I also talked to @psivaram .. We're both kind of confused on why we need to dispose this object in the first place. There's nothing to lose, other than maybe a kernel semaphore lingering for some time. It's not a connection to a database, it doesn't keep internet hostage, there's no conflict we have to avoid when another instance shows up... My proposal is to just comment the line + add a note, so other devs are not jumping on adding the disposing back. |
chrisglein
added
the
Needs: Author Feedback
ghost
added
the
no-recent-activity
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
ghost
closed this
Fix/workaround a race that occurs when the tReactInstance has been disposed on another thread while the AsyncStorage mutex is held. Thanks to my colleagues for finding this and providing a fix.
The unit test I added doesn't quite do what I want, since there's no way to synchronize without a callback. I can remove it if no one has a better idea.
I can propagate the fix to UWP if the approach is agreeable.