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 removeOfflineRecord so that it does not rely on class-level fileDeletion map #513

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

Conversation

korzonkiee
Copy link
Contributor

@korzonkiee korzonkiee commented Dec 3, 2024

Using fileDeletionMap with Rx is unsafe

Using fileDeletionMap with Rx is unsafe. Deletion in the current SDK may happen concurrently, e.g. removeOfflineRecording and deleteStoredDeviceData can be called simultanously, which can lead to ConcurrentModificationException on Android, because one thread can iterate over class-level fileDeletionMap while the other modifying it. It can lead to undeterministic behaviours. In general we think it conflicts with the reactive nature of the other features in the SDK to keep a modifiable state variable outside of the reactive flow.

Relates to

Additionally:

  1. Fixed data directory deletion. Now it works as follow: a) iterate over the parent directory of deleted offline recording (e.g. /U/0/20241203/R/094150) and add files to filesList, b) if filesList is empty, remove directory. Previously it was worked as follow: a) iterate over all offline recording files and add them to filesList, b) if filesList is empty, remove data directory. The problem with previous approach was that it checking all data directories rather than the one from which offline recording gets removed.
  2. Added some comments for better understand what the code does.
  3. Added more logs.

…eletion map

Additionally:
1. Fixed data directory deletion. Now it works as follow: a) iterate over the parent directory of deleted offline recording (e.g. /U/0/20241203/R/094150) and add files to filesList, b) if filesList is empty, remove directory. Previously it was worked as follow: a) iterate over all offline recording files and add them to filesList, b) if filesList is empty, remove data directory. The problem with previous approach was that it checking all data directories rather than the one from which offline recording gets removed.
2. Added some comments for better understand what the code does.
3. Added more logs.
@rkangast
Copy link
Contributor

rkangast commented Dec 9, 2024

Hello,

Thank you for input. We are currently fixing the "fileDeletionMap" issue, we have identified it and we will publish the fix ASAP.

The file deletion checks all data directories because we have had issues where (our internal) users have deleted the files but have not deleted the parent folders. This way we can make sure that all left over folders get deleted, too.

@rkangast
Copy link
Contributor

rkangast commented Dec 9, 2024

Hello,

Thank you for your input. We will do this change to our internal branch first and then test it and release as soon as possible.

@rkangast
Copy link
Contributor

rkangast commented Dec 9, 2024

FLOW-60203

@palmqvisti
Copy link
Contributor

This fix is merged to internal branch. Will be out in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants