Skip to content

Fix issue with re-using a shared Mutex or isolateConnectionFactory #31

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

Merged
merged 1 commit into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/src/isolate_connection_factory.dart
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ class IsolateConnectionFactory {
readOnly: readOnly,
debugName: debugName,
updates: updates.stream,
closeFunction: () {
openMutex.close();
closeFunction: () async {
await openMutex.close();
updates.close();
});
}
Expand Down Expand Up @@ -89,7 +89,7 @@ class _IsolateUpdateListener {
}

class _IsolateSqliteConnection extends SqliteConnectionImpl {
final void Function() closeFunction;
final Future<void> Function() closeFunction;

_IsolateSqliteConnection(
{required super.openFactory,
Expand All @@ -103,6 +103,6 @@ class _IsolateSqliteConnection extends SqliteConnectionImpl {
@override
Future<void> close() async {
await super.close();
closeFunction();
await closeFunction();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not awaiting here can cause the Isolate to be closed in the middle of the Mutex close process, resulting in a lock never released. Properly handling that case automatically is tricky, so for now make sure to always await closing in an Isolate.

}
}
18 changes: 2 additions & 16 deletions lib/src/mutex.dart
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ class SharedMutex implements Mutex {
closed = true;
// Wait for any existing locks to complete, then prevent any further locks from being taken out.
await _acquire();
client.fire(const _CloseMessage());
// Release the lock
_unlock();
// Close client immediately after _unlock(),
// so that we're sure no further locks are acquired.
// This also cancels any lock request in process.
Expand All @@ -201,7 +202,6 @@ class _SharedMutexServer {
Completer? unlock;
late final SerializedMutex serialized;
final Mutex mutex;
bool closed = false;

late final PortServer server;

Expand All @@ -216,11 +216,6 @@ class _SharedMutexServer {
if (arg is _AcquireMessage) {
var lock = Completer.sync();
mutex.lock(() async {
if (closed) {
// The client will error already - we just need to ensure
// we don't take out another lock.
return;
}
assert(unlock == null);
unlock = Completer.sync();
lock.complete();
Expand All @@ -231,10 +226,6 @@ class _SharedMutexServer {
} else if (arg is _UnlockMessage) {
assert(unlock != null);
unlock!.complete();
} else if (arg is _CloseMessage) {
// Unlock and close (from client side)
closed = true;
unlock?.complete();
}
}

Expand All @@ -251,11 +242,6 @@ class _UnlockMessage {
const _UnlockMessage();
}

/// Unlock and close
class _CloseMessage {
const _CloseMessage();
}

class LockError extends Error {
final String message;

Expand Down
19 changes: 19 additions & 0 deletions test/mutex_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,25 @@ void main() {
expect(result, equals(5));
}
});

test('Re-use after closing', () async {
// Test that shared locks can be opened and closed multiple times.
final mutex = SimpleMutex();
final serialized = mutex.shared;

final result = await Isolate.run(() async {
return _lockInIsolate(serialized);
});

final result2 = await Isolate.run(() async {
return _lockInIsolate(serialized);
});

await mutex.lock(() async {});

expect(result, equals(5));
expect(result2, equals(5));
});
}, timeout: const Timeout(Duration(milliseconds: 5000)));
}

Expand Down