Tool implementation doesn't handle `ParallelWaitError` about flutter HOT 6 OPEN

The new Iterable<Future>.wait and Record<Future, ...>.wait extensions are really great, particularly records allowing you to maintain type safety when waiting to resolve different types at once.

I haven't heard of these before, but they indeed seem very useful!

They can't be used inside the tool's implementation though, as the tool's error handling doesn't account for the ParallelWaitError awaiting them can throw.

Could you expand on this? What is the expected behavior here, and what is the actual behavior you see?

from flutter.

Could you expand on this? What is the expected behavior here, and what is the actual behavior you see?

Sorry for not being too specific. When running a supplied command the tool has error handling to surface any underlying errors and adapt to them differently, for example ToolExit not resulting in a crash report.

If the ToolExit results from a await (functionThatThrowsToolExit(), functionThatThrowsToolExit()).wait), the command runner will only see the ParallelWaitError that wraps it and pass that on to the user (and potentially crash reporter?).

from flutter.

Ah okay, I understand now. Thanks!

I think it makes sense to augment _handleToolError to handle ParallelWaitErrors. It's not obvious how exactly this should work since the errors list could contain any combination of ToolExits and non-ToolExits. Specifically, there are a few cases to handle:

1. cases where all of the error(s) are ToolExits,
2. cases where all of the errors(s) are not ToolExits, and
3. cases where there is some combination of ToolExits and non-ToolExits.

Case 1 is mostly a matter of figuring out what we should display to the user, which shouldn't be terribly hard.

For case 2, I think we can just let the ParallelWaitError fall through. The stack trace would be that of the first error¹, which would be enough information to triage the crash. The downside here is that the first error would effectively hide the rest of the errors. However, I don't think this would be a serious problem unless multiple non-ToolExit errors always occur together. What's more likely (in this already unlikely scenario) is that this would just result in some errors being under-reported, which I think is acceptable.

Case 3 is probably the most tricky. We can't rely on the default behavior of ParallelWaitError::stackTrace because that stack trace could be from a ToolExit, which shouldn't be the one that gets included in the crash report. One idea would be to simply throw the first non-ToolExit.

Alternatively, for cases 2 and 3, we could send crash reports for each non-ToolExit and then use only the first error for creating a crash log and the link that we display to the user. This way we don't risk one error hiding other ones.

These are just some ideas. I'd be happy to discuss and think about this further in any PR that's created to resolve this issue.

from flutter.

FWIW, if we were using Future.wait before, we were already not handling the cases above.

That is, we certainly should not crash the tool, but I think we could just show the first ToolExit (if any), and if not, show all of the errors. We could do better than that, but it would be a strict improvement over Future.wait and not feature parity.

Future.wait has different semantics, where I believe it will eagerly complete with the FIRST error, whereas (futureA, futureB).wait would collect all errors and then wrap them in a new type, ParallelWaitError.

from flutter.

FWIW, if we were using Future.wait before, we were already not handling the cases above.

That is, we certainly should not crash the tool, but I think we could just show the first ToolExit (if any), and if not, show all of the errors. We could do better than that, but it would be a strict improvement over Future.wait and not feature parity.

from flutter.

IIRC it only eagerly completes if eager: true is set, right? Otherwise it just happens to not show the other errors.

from flutter.