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.

Sign up for GitHub

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

Jump to bottom

(Will PR) Android WrappedPlayer has concurrency bugs, since it is called in multiple threads, has shared fields, but without synchronizations #1525

Closed
fzyzcjy opened this issue May 29, 2023 · 4 comments · Fixed by #1664
Closed

(Will PR) Android WrappedPlayer has concurrency bugs, since it is called in multiple threads, has shared fields, but without synchronizations #1525

fzyzcjy opened this issue May 29, 2023 · 4 comments · Fixed by #1664
Labels
bug platform-android Affects the android platform waiting for report Wait for the author to respond to the conversation

Comments

@fzyzcjy
Copy link
Contributor

fzyzcjy commented May 29, 2023 •
edited
Loading

PS. This is similar to #1524, but not the same thing.

By logging, it seems that the various methods of WrappedPlayer are called in multiple threads. However, WrappedPlayer has several fields like playing, prepared, etc, without any protection such as volatile, synchronized or anything like that. Therefore, the code seems to have bugs related to concurrency.
@fzyzcjy fzyzcjy added the bug label May 29, 2023
@fzyzcjy fzyzcjy mentioned this issue May 29, 2023
Open
@Gustl22
Copy link
Collaborator

Gustl22 commented May 30, 2023

Isn't the root cause the same as in #1524 ? What is the difference? You mean like the approach to solve the issue?

@Gustl22 Gustl22 added waiting for report Wait for the author to respond to the conversation platform-android Affects the android platform labels May 30, 2023
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 30, 2023 •
edited
Loading

Yes and no.

  1. If we enforce WrappedPlayer to be called always using the same thread (say ui thread), then 1524 is also fixed.
  2. If we only fix 1524, but still let WrappedPlayer functions be run on multi threads, then even if MediaPlayer is happy, this class (WrappedPlayer) will still be buggy
  3. The WrappedPlayer problem is looser than the MediaPlayer problem. For example, we can call WrappedPlayer using multi threads as long as using syncrhonization, while that is forbidden in 1524.

@Gustl22
Copy link
Collaborator

Gustl22 commented Jun 12, 2023

So we should solve this issue then instead of #1524. No need to fix a bug the "wrong" or not good enough way.

@Gustl22 Gustl22 mentioned this issue Jun 12, 2023
Closed
@Gustl22
Copy link
Collaborator

Gustl22 commented Oct 1, 2023

Maybe it's worth waiting for #1526

@Gustl22 Gustl22 mentioned this issue Oct 7, 2023
Merged
7 tasks
Gustl22 added a commit that referenced this issue Oct 22, 2023
@Gustl22
feat!: FramePositionUpdater & TimerPositionUpdater (#1664)
1ea9353 
# Description

- Make the position stream updates configurable via `PositionUpdater`:
- By default the position stream is updated on every new frame with help
of `FramePositionUpdater`.
- If setting the `AudioPlayer.positionUpdater` to `TimerPositionUpdater`,
the position stream is updated in the according interval.
- The execution on the separate IO thread on Android was moved to the
relevant code part in `SoundPoolPlayer`.

## Breaking Change

- [x] Yes, this is a breaking change.
- [ ] No, this is *not* a breaking change.

### Migration instructions

Removed in `audioplayers_platform_interface`:
```
AudioEventType.position
AudioEvent.position
```

## Related Issues

Closes #349
Closes #1525
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug platform-android Affects the android platform waiting for report Wait for the author to respond to the conversation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!: FramePositionUpdater & TimerPositionUpdater bluefireteam/audioplayers
3 participants
@fzyzcjy @Gustl22 and others