Baseflow / flutter_cache_manager

Generic cache manager for flutter
https://baseflow.com
MIT License
749 stars 438 forks source link

Regression: `CacheManager.removeFile` doesn't work #317

Closed sidrao2006 closed 3 years ago

sidrao2006 commented 3 years ago

🔙 Regression

CacheManager.removeFile doesn't work on the web and android since v3.0.0. However, it works well on windows. Last stable release without the bug - v2.1.2

Old (and correct) behavior

CacheManager.removeFile used to work on the web and android

Current behavior

CacheManager.removeFile doesn't work on the web and android

Reproduction steps

  1. Create an instance of CacheManager
  2. Download and cache a file using downloadFile
  3. Try removing the file

Configuration

Version: 3.x

Platform:

sidrao2006 commented 3 years ago

@renefloor I tried looking into the problem, but couldn't find anything

renefloor commented 3 years ago

@sidrao2006 I updated the example to include the removeFile option: https://github.com/Baseflow/flutter_cache_manager/commit/b1e980768a4795fa67ebff5577d0ce9216235a03

I tested this on Android and it looks like the file is removed.

Locally I also extended the method a little bit to this:

  Future<void> _removeFile() async {
    print(filePathLastFile);
    var file = io.File(filePathLastFile);
    print('File exists: ${await file.exists()}');

    // ignore: unawaited_futures
    DefaultCacheManager().removeFile(url).then((value) async {
      print('File removed');
      print('File exists: ${await file.exists()}');
    }).onError((error, stackTrace) {
      print(error);
    });
    setState(() {
      fileStream = null;
    });
  }

And I got the following result:

I/flutter (29769): /data/user/0/com.example.example/cache/libCachedImageData/25a214d0-bfbf-11eb-8d10-274131c704c3.jpg
I/flutter (29769): File exists: true
I/flutter (29769): File removed
I/flutter (29769): File exists: false

Note that the actual removal of the file is not awaited, so it might not always be removed directly:

  Future<void> _removeCachedFile(
      CacheObject cacheObject, List<int> toRemove) async {
    (...)
    if (await file.exists()) {
      unawaited(file.delete());
    }
  }

Do you have a reproducable example on Android?

sidrao2006 commented 3 years ago

@renefloor I have a test (which I can link to, if needed) which first downloads a file, checks whether it is present in cache (Using getFileFromCache) and then deletes the file and finally checks that the file is deleted... The test fails when I run it using >v2.1.2

Do you think I should use Future.delayed or something to make sure the file is deleted before testing whether it's present in the cache

sidrao2006 commented 3 years ago

Also, when I tried to create a simple example app which downloads the file, checks and deletes the file... The behaviour was rather flaky... Sometimes the file was removed and sometimes it wasn't

renefloor commented 3 years ago

Could you try adding a delay of 1 second just to see if this gives a stable result? I think it's better to make the file.delete() awaited as the benefits of speed don't really outweigh the unexpected results.

sidrao2006 commented 3 years ago

Yes, after adding a delay, the test finally passed... I agree, it is better for the file deletion to be awaited.. Should I open a PR?