handle HttpExceptions coming from ChromeTab.connect
#153978
Conversation
github-actions
bot
added
the
tool
| late final ChromeTab? chromeTab; | ||
| try { | ||
| chromeTab = await chrome.chromeConnection.getTab((ChromeTab chromeTab) { | ||
| return !chromeTab.url.startsWith('chrome-extension'); | ||
| }, retryFor: const Duration(seconds: 5)); | ||
| } on IOException { | ||
| // We were unable to unable to communicate with Chrome. | ||
| chromeTab = null; | ||
| } |
There was a problem hiding this comment.
This should already be caught by the recently added
flutter/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart
Lines 386 to 389 in 8b3cd9a
However, I prefer trying to catch exceptions as close to the thrower as we can.
There was a problem hiding this comment.
The semantics are different whether this is caught at line 386 or 616. That is to say:
- should we be calling
appFailedToStart()here? - should we be printing to STDERR the error and stacktrace?
- should we be calling the more explicit
HttpExceptionhere? - on the other hand, should we be calling the more general
IOExceptionon line 386?
I'm genuinely asking, I don't know this code at all.
There was a problem hiding this comment.
Good questions!
The semantics are different whether this is caught at line 386 or 616. That is to say:
- should we be calling
appFailedToStart()here?
This would be consistent with my principal of failing as early as possible. appFailedToStart just sets the exit code for the runner to 1:
flutter/packages/flutter_tools/lib/src/resident_runner.dart
Lines 1415 to 1417 in a9daf58
which is read by various call sites trying to attach to a device.
TL;DR yeah we probably should be calling this (though it doesn't ultimately matter since we are exiting the tool anyway).
- should we be printing to STDERR the error and stacktrace?
I don't know. I notice that we don't do so for the other exception handlers. I'm guessing the reason we don't is that such a thing might be noisy for users.
Normally I would say that users could just rerun with -v to troubleshoot, but these kind of issues tend to be difficult to reproduce, so maybe we should log to stderr.
At the risk of digressing, I think it would be cool if the tool had a mechanism to prevent error details logs to a file. Imagine this:
$flutter run -d chrome
...
blah blah
...
Failed to connect to Chrome instance.
HttpException: Connection closed before full header was received, uri = http://localhost:52097/json
More error details were logged here: path/to/logfile.txt
I think something like this already happens when the tool crashes? We could do something similar to that sans sending a crash report.
To actually answer your question: yes, I've added some extra logging here. However, the tradeoff, again, is that users might waste their time reading noise they don't care about. Consider this, for example (but with a much, much longer stack trace):
Waiting for connection from debug service on Chrome... 9.7s
HttpException: uh oh!
#0 ResidentWebRunner.attach (package:flutter_tools/src/isolated/resident_web_runner.dart:616:9)
<asynchronous suspension>
#1 asyncGuard.<anonymous closure> (package:flutter_tools/src/base/async_guard.dart:111:24)
<asynchronous suspension>
Failed to connect to Chrome instance.
- should we be calling the more explicit
HttpExceptionhere?
If we wish to be more conservative (which is worth considering since this would be CP'd to stable), then sure. In fact, this is probably the way to go (we probably wouldn't want to catch FileSystemExceptions, for example—as unlikely as they may be). Maybe we should also catch SocketException and whatnot here as well, but I'm content to wait until it shows up in crash reports before going ahead and fixing that.
- on the other hand, should we be calling the more general
IOExceptionon line 386?I'm genuinely asking, I don't know this code at all.
Probably not, see my previous point.
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
This is beyond me, but at a high level it looks like the kind of thing that would catch this :)
andrewkolos
force-pushed
the
handle-exceptions-from-connecting-to-chrome
branch
2 times, most recently
from
26add34 to
cee7482
Compare
Co-authored-by: Christopher Fujino <[email protected]>
This reverts commit 01b9268.
This reverts commit 63021c0.
andrewkolos
force-pushed
the
handle-exceptions-from-connecting-to-chrome
branch
from
cee7482 to
bf8d3ef
Compare
andrewkolos
added
the
autosubmit
|
auto label is removed for flutter/flutter/153978, due to - The status or check suite Linux customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
andrewkolos
added
the
autosubmit
Fixes flutter#153972 (unless the cause of flutter#153064 (comment) happens to also prevent this fix from working). In this PR, I've looked for all non-test call sites of `ChromeConnection.getTabs` and made sure are all wrapped in `try` blocks that handle `IOException` (`HttpException` is what we see in crash reporting, but I figure any `IOException` might as well be the same for all intents and purposes). I plan on cherry-picking this the stable branch.
…ction.getTab` (#154380) This is a custom hotfix patch to fix #154366 and #153064 on the stable channel. The reason for a custom patch rather than a bot-created PR is that the fixes on master were done by two sequential PRs (#153978 and #154366). This CP patch is also a subset of those changes (calls to Chrome when shutting down at the end of `flutter run -d chrome`).
3 participants
Fixes #153972 (unless the cause of #153064 (comment) happens to also prevent this fix from working).
In this PR, I've looked for all non-test call sites of
ChromeConnection.getTabsand made sure are all wrapped intryblocks that handleIOException(HttpExceptionis what we see in crash reporting, but I figure anyIOExceptionmight as well be the same for all intents and purposes).I plan on cherry-picking this the stable branch.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.