Catch exceptions in unawaited Futures
#1938
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
annagrin
reviewed
Feb 4, 2023
This was referenced Feb 6, 2023
elliette
commented
Feb 7, 2023
Comment on lines
+80
to
+107
| /// Throws an [wip.ExceptionDetails] object if `exceptionDetails` is present on the | ||
| /// result. | ||
| void handleErrorIfPresent(wip.WipResponse? response, {String? evalContents}) { | ||
| final result = response?.result; | ||
| if (result == null) return; | ||
| if (result.containsKey('exceptionDetails')) { | ||
| throw ChromeDebugException( | ||
| result['exceptionDetails'] as Map<String, dynamic>, | ||
| evalContents: evalContents, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// Returns result contained in the response. | ||
| /// Throws an [wip.ExceptionDetails] object if `exceptionDetails` is present on the | ||
| /// result or the result is null. | ||
| Map<String, dynamic> getResultOrHandleError(wip.WipResponse? response, | ||
| {String? evalContents}) { | ||
| handleErrorIfPresent(response, evalContents: evalContents); | ||
| final result = response?.result?['result']; | ||
| if (result == null) { | ||
| throw ChromeDebugException( | ||
| {'text': 'null result from Chrome Devtools'}, | ||
| evalContents: evalContents, | ||
| ); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Had to move these out of shared because they also have downstream dependencies on dart:io
annagrin
reviewed
Feb 7, 2023
| @@ -134,7 +135,7 @@ class BatchedExpressionEvaluator extends ExpressionEvaluator { | |||
| createError(ErrorKind.internal, 'No batch result object ID.'); | |||
| request.completer.complete(error); | |||
| } else { | |||
| unawaited(_debugger | |||
| safeUnawaited(_debugger | |||
There was a problem hiding this comment.
I wonder what happens on failure inside safeUnawaited - will .then below be executed and with what result. I think we might need to add an onError(e,s) function as a parameter to safeUnawaited so the user (batched expression evaluator in this case) can decide what to do with the error. In case of expression evaluation we should complete with an error.
There was a problem hiding this comment.
Updated so we are now completing with error
dwds/lib/src/utilities/shared.dart
Outdated
| ); | ||
| } | ||
| return result; | ||
| void safeUnawaited(Future<void> future) { |
There was a problem hiding this comment.
Suggestion, so we an safely use this in batched expression evaluator:
void safeUnawaited(Future<void> future, {
void Function(Exception, StackTrace)? onError,
}) {
onError ??= (e,s) =>
_logger.warning('Error in unawaited Future:', e, s);
unawaited(future.catchError(onError));
}
There was a problem hiding this comment.
SG, made that change!
annagrin
approved these changes
Feb 7, 2023
| 'Got result out of a batch for ${request.expression}: $result'); | ||
| request.completer.complete(result); | ||
| }), | ||
| onError: (error, _) => request.completer.completeError(error), |
There was a problem hiding this comment.
Let's also pass the stackTrace to the completeError for diagnostic purposes. We will report this error in _getEvaluationResult in chrome_proxy_service.dart. Thanks for fixing this!
Comment on lines
+80
to
+107
| /// Throws an [wip.ExceptionDetails] object if `exceptionDetails` is present on the | ||
| /// result. | ||
| void handleErrorIfPresent(wip.WipResponse? response, {String? evalContents}) { | ||
| final result = response?.result; | ||
| if (result == null) return; | ||
| if (result.containsKey('exceptionDetails')) { | ||
| throw ChromeDebugException( | ||
| result['exceptionDetails'] as Map<String, dynamic>, | ||
| evalContents: evalContents, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// Returns result contained in the response. | ||
| /// Throws an [wip.ExceptionDetails] object if `exceptionDetails` is present on the | ||
| /// result or the result is null. | ||
| Map<String, dynamic> getResultOrHandleError(wip.WipResponse? response, | ||
| {String? evalContents}) { | ||
| handleErrorIfPresent(response, evalContents: evalContents); | ||
| final result = response?.result?['result']; | ||
| if (result == null) { | ||
| throw ChromeDebugException( | ||
| {'text': 'null result from Chrome Devtools'}, | ||
| evalContents: evalContents, | ||
| ); | ||
| } | ||
| return result; | ||
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up work to flutter/flutter#119084
DWDS was causing
flutter_toolsto crash due to using a null-check assertion on a null value in an unawaitedFuture.This PR:
safeUnawaitedwhich catches and logs any exceptions thrown in the unawaitedFutureunawaitedin DWDS tosafeUnawaitedinstead