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

Path exception error while trying to delete cached files #1194

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

DGoiana
Copy link
Collaborator

@DGoiana DGoiana commented Mar 6, 2024

Closes #1177

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@DGoiana DGoiana changed the base branch from develop to master March 6, 2024 22:10
@DGoiana DGoiana changed the title Fix/path access exception Path exception error while trying to delete cached files Mar 7, 2024
@limwa

This comment has been minimized.

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Merging #1194 (ee0fec0) into master (be1c3ae) will increase coverage by 1%.
Report is 2 commits behind head on master.
The diff coverage is 0%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1194   +/-   ##
======================================
+ Coverage      17%     17%   +1%     
======================================
  Files         229     229           
  Lines        6974    6970    -4     
======================================
  Hits         1144    1144           
+ Misses       5830    5826    -4     

Copy link
Member

@limwa limwa left a comment

Choose a reason for hiding this comment

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

I've written some suggestions for the deletion. Do we need to also delete the folders? Here, I've ignored the folders and focused on only deleting files, since taking care of folders is much more complicated (folders need to be deleted with recursive true, unless they are empty).

Either way, really good work here and thanks for being so quick!

uni/lib/controller/cleanup.dart Outdated Show resolved Hide resolved
uni/lib/controller/cleanup.dart Outdated Show resolved Hide resolved
@limwa
Copy link
Member

limwa commented Mar 7, 2024

Something else that just popped into my mind is that the database connections need to be closed before the corresponding database files are deleted (if you want, you don't even need to call the deleteLectures, ... methods because the files will be deleted afterwards anyways!). You would need to ensure that no connection can be opened while the file deletion is ongoing.

Another way is to not close the database connections and not delete the database files (for instance, not deleting files that end in .db, or something else)

Which one do you prefer?

@DGoiana
Copy link
Collaborator Author

DGoiana commented Mar 7, 2024

Something else that just popped into my mind is that the database connections need to be closed before the corresponding database files are deleted (if you want, you don't even need to call the deleteLectures, ... methods because the files will be deleted afterwards anyways!). You would need to ensure that no connection can be opened while the file deletion is ongoing.

Another way is to not close the database connections and not delete the database files (for instance, not deleting files that end in .db, or something else)

Which one do you prefer?

I am avoiding the deletion of .db files to not force any connection lock to the databases.

@limwa
Copy link
Member

limwa commented Mar 7, 2024

Sounds good! I'll review this PR later today!

@limwa limwa force-pushed the fix/path-access-exception branch from d44a9b9 to 08baa2f Compare March 7, 2024 17:40
uni/lib/controller/cleanup.dart Outdated Show resolved Hide resolved
@DGoiana DGoiana force-pushed the fix/path-access-exception branch from 08baa2f to d44a9b9 Compare March 8, 2024 20:07
Copy link
Member

@limwa limwa left a comment

Choose a reason for hiding this comment

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

Well done! 🚀

@limwa limwa enabled auto-merge March 8, 2024 20:12
@limwa limwa disabled auto-merge March 8, 2024 20:14
@limwa limwa enabled auto-merge March 8, 2024 20:14
@limwa limwa merged commit 577444f into master Mar 8, 2024
6 checks passed
@limwa limwa deleted the fix/path-access-exception branch March 8, 2024 21:15
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.

PathAccessException in cleanup.dart
3 participants