Baseflow / flutter_cache_manager

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

_removeCachedFile Null check operator used on a null value #297

Closed creativecreatorormaybenot closed 3 years ago

creativecreatorormaybenot commented 3 years ago

🐛 Bug Report

When I call removeFile, the plugin throws the following exception.

[ERROR:flutter/lib/ui/ui_dart_state.cc(186)] Unhandled Exception: Null check operator used on a null value
E/flutter (13327): #0      CacheStore._removeCachedFile (package:flutter_cache_manager/src/cache_store.dart:171:32)
E/flutter (13327): #1      CacheStore.removeCachedFile (package:flutter_cache_manager/src/cache_store.dart:163:15)
E/flutter (13327): <asynchronous suspension>
E/flutter (13327): #2      CacheManager.removeFile (package:flutter_cache_manager/src/cache_manager.dart:276:7)
E/flutter (13327): <asynchronous suspension>

Expected behavior

The cached file should be removed but is not (instead, the exception is thrown).

Reproduction steps

  1. Call DefaultCacheManager().removeFile(fileKey);.

Then the exception is thrown.

I assume this has to be the plugin's fault (looking at the source code) because the object seems to be valid according to this check:

https://github.com/Baseflow/flutter_cache_manager/blob/c869918f891144c7472e0187b615012727176ef9/flutter_cache_manager/lib/src/cache_manager.dart#L270

Then here it fails (line 171 in cache_store.dart:

    toRemove.add(cacheObject.id!);

Note that it fails with the null safe version, which I cannot seem to find here on GitHub.

Configuration

Version: flutter_cache_manager: ^3.0.0-nullsafety.1

Platform:

renefloor commented 3 years ago

Thanks for the good report. The null safe version is this PR: #278

creativecreatorormaybenot commented 3 years ago

Oh I see, thanks @renefloor 🙏

davidmartos96 commented 3 years ago

@renefloor I've encountered with this as well. I'd say the issue is that the object id is null, no that the cache object itself is null. Maybe a check for the id not null would suffice.

renefloor commented 3 years ago

@renefloor I've encountered with this as well. I'd say the issue is that the object id is null, no that the cache object itself is null. Maybe a check for the id not null would suffice.

I don't think so. This is the code:

    final cacheObject = await _store.retrieveCacheData(key);
    if (cacheObject != null) {
      await _store.removeCachedFile(cacheObject);
    }

When the key is in the cache store the object id should be set. If the key is not in the database the cacheObject should be null. We'll have to check how it is possible there is a cacheObject returned without an id. I can't think of a case that that should happen, but I didn't look at the code yet how it is possible.

davidmartos96 commented 3 years ago

@renefloor Could it be that the object stored in the memCache doesn't have the id obtained from the database?

Look at this code:

image

From what I've seen, putFile gets called with newly created objects, without id. The id is assigned after it gets cached in memory. This exception doesn't happen the first time the removeFile function gets called, so the memory cache could be related.

renefloor commented 3 years ago

Ah that makes sense. Do you use this on web?

davidmartos96 commented 3 years ago

@renefloor No, it's mobile only

davidmartos96 commented 3 years ago

@renefloor But I have somewhat of a custom cache logic, I don't know if that could trigger this case case.

Future<File> getMyFileFromCache(String url, {@required bool fresh}) async {
  final cache = sl<MyCacheManager>();
  if (fresh) {
    await cache.removeFile(url);
  }
  // This here it's an extension method that uses `getFileFromCache` and `downloadFile` with a custom logic
  final File f = await cache.getValidFileOrFresh(url);
  return f;
}
renefloor commented 3 years ago

I still don't really understand why it happens, but I did this fix in 3.0.0-nullsafety.3