Skip to content
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

Fix TapGestureDetector mutex is not locked error on Touchscreen #824

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Sep 18, 2023

Fixes JetBrains/compose-multiplatform#3655

The issue is introduced after migration to awaitPointerEventScope, this code is indeed has a race condition, as it was assumed in the TODO. When we send Press/Release without any moves - we go via this path:

// awaitPointerEventScope isn't called until it receives the first event
awaitPointerEventScope {
  // in the beginning of the method we have 2 events already in the queue

  launch { pressScope.reset() } // launch async, reset isn't called
  val down = awaitPress // don't suspend here, because we already have Press
  awaitReleaseOrCancelled() // don't suspend here, because we already have Release
  pressScope.release() // crash, because `pressScope.reset()` isn't called yet
}

To avoid races, we should always access pressScope in launch (this way the calls maintain order).

The same approach is used in the original TapGestureDetector.kt (the change in commit 32de9dd)

Also, the rest of the code doesn't have races with pressScope calls.

Testing

  • haven't reproduced with mouse
  • reproduced with a simple test (added to OnClickTest)

Fixes JetBrains/compose-multiplatform#3655

The issue is introduced after migration to `awaitPointerEventScope`, this code is indeed has a race condition, as it was assumed in the TODO. When we send Press/Release without any moves - we go via this path:
```
// awaitPointerEventScope isn't called until it receives the first event
awaitPointerEventScope {
  // in the beginning of the method we have 2 events already in the queue

  launch { pressScope.reset() } // launch async, reset isn't called
  val down = awaitPress // don't suspend here, because we already have Press
  awaitReleaseOrCancelled() // don't suspend here, because we already have Release
  pressScope.release() // crash, because `pressScope.reset()` isn't yet called
}
```

To avoid races, we should always access `pressScope` in `launch` (this way calls maintain order).

The same approach is used in the original `TapGestureDetector.kt` (the change in commit 32de9dd)

Also, the rest of the code doesn't have races with `pressScope` calls.

## Testing
- haven't reproduced with mouse
- reproduced with a simple test (added to OnClickTest)
@igordmn igordmn merged commit f0bc549 into jb-main Sep 19, 2023
3 checks passed
@igordmn igordmn deleted the igor.demin/fix-onclick-crash branch September 19, 2023 12:29
elijah-semyonov pushed a commit that referenced this pull request Sep 21, 2023
Fixes JetBrains/compose-multiplatform#3655

The issue is introduced after migration to `awaitPointerEventScope`,
this code is indeed has a race condition, as it was assumed in the TODO.
When we send Press/Release without any moves - we go via this path:
```
// awaitPointerEventScope isn't called until it receives the first event
awaitPointerEventScope {
  // in the beginning of the method we have 2 events already in the queue

  launch { pressScope.reset() } // launch async, reset isn't called
  val down = awaitPress // don't suspend here, because we already have Press
  awaitReleaseOrCancelled() // don't suspend here, because we already have Release
  pressScope.release() // crash, because `pressScope.reset()` isn't called yet
}
```

To avoid races, we should always access `pressScope` in `launch` (this
way the calls maintain order).

The same approach is used in the original `TapGestureDetector.kt` (the
change in commit 32de9dd)

Also, the rest of the code doesn't have races with `pressScope` calls.

## Testing
- haven't reproduced with mouse
- reproduced with a simple test (added to OnClickTest)
Walingar pushed a commit that referenced this pull request Sep 25, 2023
Fixes JetBrains/compose-multiplatform#3655

The issue is introduced after migration to `awaitPointerEventScope`,
this code is indeed has a race condition, as it was assumed in the TODO.
When we send Press/Release without any moves - we go via this path:
```
// awaitPointerEventScope isn't called until it receives the first event
awaitPointerEventScope {
  // in the beginning of the method we have 2 events already in the queue

  launch { pressScope.reset() } // launch async, reset isn't called
  val down = awaitPress // don't suspend here, because we already have Press
  awaitReleaseOrCancelled() // don't suspend here, because we already have Release
  pressScope.release() // crash, because `pressScope.reset()` isn't called yet
}
```

To avoid races, we should always access `pressScope` in `launch` (this
way the calls maintain order).

The same approach is used in the original `TapGestureDetector.kt` (the
change in commit 32de9dd)

Also, the rest of the code doesn't have races with `pressScope` calls.

## Testing
- haven't reproduced with mouse
- reproduced with a simple test (added to OnClickTest)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants