Fix future type to allow returning null in catchError

[davidmartos96]

Hello!
I've recently encountered with an issue when awaiting the close of a PostgresPool
After some debugging I found that the issue comes from the usage of catchError + null return when joining the tasks.

executor/lib/src/executor_impl.dart

Line 129 in 833ce15

futures.add(item.result.future.catchError((_) async => null));

The type of the task is R, but that doesn't mean it can accept null as a type, so it fails at runtime with the following error.
Invalid argument(s) (onError): The error handler of Future.catchError must return a value of the future's type

Here is a test that breaks in current version of executor.

        final executor = Executor(concurrency: 2);
        for (var i = 0; i < 10; i++) {
          // Non-nullable int as a type
          // This mimics the usage of executor in postgres_pool, 
          // which are also non-nullable types
          executor.scheduleTask<int>(() async {
            await Future.delayed(Duration(microseconds: i * 10));
            await Future<Null>.microtask(() => throw Exception());

            // Some arbitrary value to match the task type
            return 1;
          });
        }
        await executor.join(withWaiting: true);
        await executor.close();

This PR adds a few tests to check for non-null and nullable task types behavior and fixes the issue.

[isoos]

I'm not entirely convinced that the right fix is the current one in the PR. Would you please try out the List<Object?> return type as suggested, whether it is still an issue with postgres?

[isoos]

This feels like something that shouldn't really happen, or rather: something that we shouldn't write. I'd think (v) => v as R would be sufficient, but then it would be a no-op, wouldn't it?

[davidmartos96]

result.future is R? so removing the then would not compile. I've updated the code with simply v as R

[isoos]

The method ends with Future.wait which returns with a list:
https://api.dart.dev/stable/2.19.1/dart-async/Future/wait.html

Maybe the package:postgres issue comes from the lack of type annotation here, and the return type should be Future<List<Object?>> instead. It seems to be better to expose the return type here, than to make type casts in the schedule, which 3-6 months from now would seem rather mysterious.

[davidmartos96]

I leave that to your decision. I typed it as void because the output of join is not used.
The function could also be marked as async and instead of returning the future we can do await Future.wait(...) so that the output is void

[davidmartos96]

I'm not entirely convinced that the right fix is the current one in the PR. Would you please try out the List<Object?> return type as suggested, whether it is still an issue with postgres?

I don't think it's a problem with postgres_pool, rather the pool it's using the executor with non-null typed tasks, which when dealing with catchErrors, fail

About the List<Object?>, would you like to make that the output type for join?

[isoos]

About the List<Object?>, would you like to make that the output type for join?

I think that is the key here: we need to return a list, otherwise it would be a breaking change (not on the explicit API, but in behavior). It also has a high chance that this is all we need to fix the issue you have encountered, and revert the changes in the other parts (notably the then call).

[isoos]

published, thank you for the change and the patient explanation!

[davidmartos96]

Thank you! The issue was kind of hard to explain with words 😅