Skip to content

Commit

Permalink
fix(storage): Task.cancel() method wasn't properly updating `task.s…
Browse files Browse the repository at this point in the history 
…napshot` and `cancel()` wasn't working in certain conditions. (#12322)
  • Loading branch information
@russellwheatley
russellwheatley committed Feb 22, 2024
1 parent bd2a678 commit c3ca5d1
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 54 deletions.
19 changes: 16 additions & 3 deletions ...roid/src/main/java/io/flutter/plugins/firebase/storage/TaskStateChannelStreamHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import androidx.annotation.Nullable;
import com.google.firebase.storage.FirebaseStorage;
import com.google.firebase.storage.StorageException;
import com.google.firebase.storage.StorageTask;
import io.flutter.plugin.common.EventChannel.EventSink;
import io.flutter.plugin.common.EventChannel.StreamHandler;
Expand All @@ -21,6 +22,7 @@ public class TaskStateChannelStreamHandler implements StreamHandler {
private final String TASK_STATE_NAME = "taskState";
private final String TASK_APP_NAME = "appName";
private final String TASK_SNAPSHOT = "snapshot";
private final String TASK_ERROR = "error";

public TaskStateChannelStreamHandler(
FlutterFirebaseStorageTask flutterTask,
Expand Down Expand Up @@ -71,7 +73,17 @@ public void onListen(Object arguments, EventSink events) {
Map<String, Object> event = getTaskEventMap(null, null);
event.put(
TASK_STATE_NAME,
GeneratedAndroidFirebaseStorage.PigeonStorageTaskState.CANCELED.index);
// We use "Error" state as we synthetically update snapshot cancel state in cancel() method in Dart
// This is also inline with iOS which doesn't have a cancel state, only failure
GeneratedAndroidFirebaseStorage.PigeonStorageTaskState.ERROR.index);
// We need to pass an exception that the task was canceled like the other platforms
Map<String, Object> syntheticException = new HashMap<>();
syntheticException.put(
"code", FlutterFirebaseStorageException.getCode(StorageException.ERROR_CANCELED));
syntheticException.put(
"message",
FlutterFirebaseStorageException.getMessage(StorageException.ERROR_CANCELED));
event.put(TASK_ERROR, syntheticException);
events.success(event);
flutterTask.notifyCancelObjects();
flutterTask.destroy();
Expand All @@ -90,7 +102,8 @@ public void onListen(Object arguments, EventSink events) {

@Override
public void onCancel(Object arguments) {
// Task already destroyed, do nothing.
if (!androidTask.isCanceled()) androidTask.cancel();
if (!flutterTask.isDestroyed()) flutterTask.destroy();
}

private Map<String, Object> getTaskEventMap(
Expand All @@ -101,7 +114,7 @@ private Map<String, Object> getTaskEventMap(
arguments.put(TASK_SNAPSHOT, FlutterFirebaseStorageTask.parseTaskSnapshot(snapshot));
}
if (exception != null) {
arguments.put("error", FlutterFirebaseStoragePlugin.getExceptionDetails(exception));
arguments.put(TASK_ERROR, FlutterFirebaseStoragePlugin.getExceptionDetails(exception));
}
return arguments;
}
Expand Down
31 changes: 20 additions & 11 deletions packages/firebase_storage/firebase_storage/ios/Classes/FLTFirebaseStoragePlugin.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -691,17 +691,26 @@ - (void)setState:(FLTFirebaseStorageTaskState)state
[task removeObserverWithHandle:failureHandle];
completion(NO, nil);
}];
failureHandle =
[task observeStatus:FIRStorageTaskStatusFailure
handler:^(FIRStorageTaskSnapshot *snapshot) {
[task removeObserverWithHandle:successHandle];
[task removeObserverWithHandle:failureHandle];
if (snapshot.error && snapshot.error.code == FIRStorageErrorCodeCancelled) {
completion(YES, [FLTFirebaseStoragePlugin parseTaskSnapshot:snapshot]);
} else {
completion(NO, nil);
}
}];
failureHandle = [task
observeStatus:FIRStorageTaskStatusFailure
handler:^(FIRStorageTaskSnapshot *snapshot) {
[task removeObserverWithHandle:successHandle];
[task removeObserverWithHandle:failureHandle];

if (snapshot.error && snapshot.error && snapshot.error.userInfo) {
// For UploadTask, the error code is found in the userInfo
// We use it to match this:
// https://github.com/firebase/firebase-ios-sdk/blob/main/FirebaseStorage/Sources/StorageError.swift#L37
NSNumber *responseErrorCode = snapshot.error.userInfo[@"ResponseErrorCode"];
if ([responseErrorCode integerValue] == FIRStorageErrorCodeCancelled ||
snapshot.error.code == FIRStorageErrorCodeCancelled) {
completion(YES, [FLTFirebaseStoragePlugin parseTaskSnapshot:snapshot]);
return;
}
}

completion(NO, nil);
}];
[task cancel];
} else {
completion(NO, nil);
Expand Down
9 changes: 8 additions & 1 deletion packages/firebase_storage/firebase_storage/ios/Classes/FLTTaskStateChannelStreamHandler.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,17 @@ - (FlutterError *)onListenWithArguments:(id)arguments eventSink:(FlutterEventSin
failureHandle =
[_task observeStatus:FIRStorageTaskStatusFailure
handler:^(FIRStorageTaskSnapshot *snapshot) {
NSError *error = snapshot.error;
// If the snapshot.error is "unknown" and there is an underlying error, use
// this. For UploadTasks, the correct error is in the underlying error.
if (snapshot.error.code == FIRStorageErrorCodeUnknown &&
snapshot.error.userInfo[@"NSUnderlyingError"] != nil) {
error = snapshot.error.userInfo[@"NSUnderlyingError"];
}
events(@{
@"taskState" : @(PigeonStorageTaskStateError),
@"appName" : snapshot.reference.storage.app.name,
@"error" : [FLTFirebaseStoragePlugin NSDictionaryFromNSError:snapshot.error],
@"error" : [FLTFirebaseStoragePlugin NSDictionaryFromNSError:error],
});
}];
pausedHandle =
Expand Down
50 changes: 30 additions & 20 deletions ...orage/firebase_storage_platform_interface/lib/src/method_channel/method_channel_task.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,26 +35,31 @@ abstract class MethodChannelTask extends TaskPlatform {
try {
await for (final events in nativePlatformStream) {
final taskState = TaskState.values[events['taskState']];

if (taskState == TaskState.error) {
_didComplete = true;
final errorMap = Map<String, dynamic>.from(events['error']);
final code = errorMap['code'];

final exception = FirebaseException(
plugin: 'firebase_storage',
code: errorMap['code'],
code: code,
message: errorMap['message'],
);
_snapshot = MethodChannelTaskSnapshot(
storage,
taskState,
// We use previous snapshot data as errors from native do not provide snapshot data.
{
'path': path,
'bytesTransferred': _snapshot.bytesTransferred,
'totalBytes': _snapshot.totalBytes,
'metadata': _snapshot.metadata
},
);
if (code != 'canceled') {
// If the task was canceled, we keep the previous snapshot data with canceled state.
_snapshot = MethodChannelTaskSnapshot(
storage,
taskState,
// We use previous snapshot data as errors from native do not provide snapshot data.
{
'path': path,
'bytesTransferred': _snapshot.bytesTransferred,
'totalBytes': _snapshot.totalBytes,
'metadata': _snapshot.metadata
},
);
}
_exception = exception;
_completer?.completeError(exception);
if (_userListening) {
// If the user is listening to the stream, yield the error. Otherwise, it results in an unhandled exception.
Expand All @@ -73,9 +78,12 @@ abstract class MethodChannelTask extends TaskPlatform {
break;
}

if (taskState == TaskState.success ||
taskState == TaskState.running ||
taskState == TaskState.paused) {
if ((taskState == TaskState.success ||
taskState == TaskState.running ||
taskState == TaskState.paused)
// Required for android which fires another event when already cancelled
&&
snapshot.state != TaskState.canceled) {
MethodChannelTaskSnapshot snapshot = MethodChannelTaskSnapshot(
storage,
taskState,
Expand Down Expand Up @@ -138,8 +146,6 @@ abstract class MethodChannelTask extends TaskPlatform {

Object? _exception;

late StackTrace _stackTrace;

bool _didComplete = false;

Completer<TaskSnapshotPlatform>? _completer;
Expand Down Expand Up @@ -169,11 +175,15 @@ abstract class MethodChannelTask extends TaskPlatform {
if (_didComplete && _exception == null) {
return Future.value(snapshot);
} else if (_didComplete && _exception != null) {
return catchFuturePlatformException(_exception!, _stackTrace);
return catchFuturePlatformException(_exception!, StackTrace.current);
} else {
// Call _stream.last to trigger the stream initialization, in case it hasn't been.
// ignore: unawaited_futures
_stream.last;
_stream.last.catchError((_) {
// We ignore the exception here, stream exceptions are propagated in the stream handler above
// This causes unhandled exceptions when task.listen & onComplete are used together.
return Future.value(snapshot);
});
_completer ??= Completer<TaskSnapshotPlatform>();
return _completer!.future;
}
Expand Down
38 changes: 21 additions & 17 deletions tests/integration_test/firebase_storage/task_e2e.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';
import 'dart:convert';
import 'dart:io';

Expand Down Expand Up @@ -230,17 +231,17 @@ void setupTaskTests() {

Future<void> _testCancelTask() async {
List<TaskSnapshot> snapshots = [];
FirebaseException? streamError;
expect(task.snapshot.state, TaskState.running);
final Completer<FirebaseException> errorReceived =
Completer<FirebaseException>();

task.snapshotEvents.listen(
(TaskSnapshot snapshot) {
snapshots.add(snapshot);
},
onError: (error) {
streamError = error;
errorReceived.complete(error);
},
cancelOnError: true,
);

bool canceled = await task.cancel();
Expand All @@ -257,8 +258,11 @@ void setupTaskTests() {

expect(task.snapshot.state, TaskState.canceled);

// Need to wait for error to be received before checking
final streamError = await errorReceived.future;

expect(streamError, isNotNull);
expect(streamError!.code, 'canceled');
expect(streamError.code, 'canceled');
// Expecting there to only be running states, canceled should not get sent as an event.
expect(
snapshots.every((snapshot) => snapshot.state == TaskState.running),
Expand All @@ -269,24 +273,24 @@ void setupTaskTests() {
test(
'successfully cancels download task',
() async {
if (!kIsWeb) {
file = await createFile('ok.jpeg');
task = downloadRef.writeToFile(file);
} else {
task = downloadRef
.putBlob(createBlob('some content to write to blob'));
}

file = await createFile('ok.jpeg', largeString: 'A' * 20000000);
task = downloadRef.writeToFile(file);
await _testCancelTask();
},
// There's no DownloadTask on web.
skip: kIsWeb,
retry: 2,
);

test('successfully cancels upload task', () async {
task = uploadRef.putString('This is an upload task!');
await _testCancelTask();
});
test(
'successfully cancels upload task',
() async {
task = uploadRef.putString('A' * 20000000);
await _testCancelTask();
},
retry: 2,
);
},
skip: true, // Cancel still cannot get correct result in e2e test.
);

group('snapshotEvents', () {
Expand Down
4 changes: 2 additions & 2 deletions tests/integration_test/firebase_storage/test_utils.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ const int testEmulatorPort = 9199;

// Creates a test file with a specified name to
// a locally directory
Future<File> createFile(String name) async {
Future<File> createFile(String name, { String? largeString }) async {
final Directory systemTempDir = Directory.systemTemp;
final File file = await File('${systemTempDir.path}/$name').create();
await file.writeAsString(kTestString);
await file.writeAsString(largeString ?? kTestString);
return file;
}

Expand Down

0 comments on commit c3ca5d1

Please sign in to comment.