Do exponential backoff for all exceptions in VMService::defaultOpenChannel. #16785
Conversation
…annel We were trying to only catch WebSocketException, but in fact SocketException can be thrown as well.
|
|
|
Feel free to land as soon as Travis is happy. Should be a matter of updating |
| @@ -53,7 +53,7 @@ Future<StreamChannel<String>> _defaultOpenChannel(Uri uri) async { | |||
| attempts += 1; | |||
| try { | |||
| socket = await io.WebSocket.connect(uri.toString()); | |||
| } on io.WebSocketException catch(e) { | |||
| } catch (e) { | |||
| printTrace('Exception attempting to connect to observatory: $e'); | |||
There was a problem hiding this comment.
This means if the error was something like a CompilerError, we'll retry anyway, which is a bit weird. But I guess we're seeing a lot of different kinds of errors here?
There was a problem hiding this comment.
We can get both SocketException and WebSocketException, which for some reason don't extend one another. So I could have added two catch blocks, but I went with just a global catch. Lemme know if you think I should change it.
There was a problem hiding this comment.
As a general rule I prefer not doing global catches because they tend to swallow real errors. In this case it's not a huge deal since the real errors will still get noisily printed to the console and eventually result in a failure. I'll leave it up to you which way you want to go on this.
There was a problem hiding this comment.
What we really want here is Java multi-catch ability:
} on io.SocketExceptiojn, io.WebSocketException catch (e) {
...
}But that doesn't exist. Given the limited scope of what's in this try/catch, I'm fine with leaving it as-is, but I don't feel strongly either way.
…annel. (flutter#16785) We were trying to only catch WebSocketException, but in fact SocketException can be thrown as well.
We were trying to only catch WebSocketException, but in fact
SocketException can be thrown as well.