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

fix: resolve thread safety issue in CallRecordDatabase #475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: resolve thread safety issue in CallRecordDatabase #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@

- Updated the Twilio Voice iOS SDK dependency to `6.12.1`.

## Fixes

### Platform Specific Fixes

#### Android

- Fix thread safety issue in the `CallRecordDatabase`

1.3.0 (Dec 10, 2024)
====================
Expand Down
8 changes: 6 additions & 2 deletions android/src/main/java/com/twiliovoicereactnative/CallRecordDatabase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,18 @@ public void clear() {

public CallRecord get(final CallRecord record) {
try {
return callRecordList.get(callRecordList.indexOf(record));
synchronized (callRecordList) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Under what condition are there multiple threads accessing the call record database?

Copy link
Author

Choose a reason for hiding this comment

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

The issue we ran into was rejecting or accepting a call at the same time that the calling end hangs up, but there are probably others.

This becomes extra problematic because it results in a null return type, which from most of the call points of get or remove will result in an exception that crashes the app since they are wrapped in Objects.requireNonNull. This does not entirely address that problem, but is a start.

Copy link
Collaborator

@afalls-twilio afalls-twilio Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jhutchins Are you using new-arch? I ask because historically all invocations were coming from the same thread. IE. the main thread, and therefore impossible for a remove to happen at the exact same time as a addition. Thank you for providing the use-case and we will attempt to recreate the issue you are experiencing...

Copy link
Author

Choose a reason for hiding this comment

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

No, we are explicitly disabling new-arch in our gradle.properties file.

Screenshot 2025-02-12 at 2 53 09 AM

In our case, we are answering the call from the JavaScript and in that case you can see that the various methods within CallRecordDatabase are accessed from different threads.

Screenshot 2025-02-12 at 3 13 29 AM
Screenshot 2025-02-12 at 3 14 06 AM

The same will be true when you hang up using a button on the active call screen, there will be a get from the mqt_native_modules thread which could cause an issue if both ends of a call try to hang up at the same time.

return callRecordList.get(callRecordList.indexOf(record));
}
} catch (IndexOutOfBoundsException e) {
return null;
}
}
public CallRecord remove(final CallRecord record) {
try {
return callRecordList.remove(callRecordList.indexOf(record));
synchronized (callRecordList) {
return callRecordList.remove(callRecordList.indexOf(record));
}
} catch (IndexOutOfBoundsException e) {
return null;
}
Expand Down