Skip to content

Protect against NPE caused by null devtoolsServer#1940

Merged
kenzieschmoll merged 1 commit into
flutter:masterfrom
kenzieschmoll:server
May 7, 2020
Merged

Protect against NPE caused by null devtoolsServer#1940
kenzieschmoll merged 1 commit into
flutter:masterfrom
kenzieschmoll:server

Conversation

@kenzieschmoll

Copy link
Copy Markdown
Member

@DanTup we were seeing exceptions in g3 because devToolsServer was null ("devtools server not available (204)" printed to console in this case).

This will fix the NPE but do we expect `devToolsServer to be null?

@devoncarew devoncarew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I believe that devToolsServer will be null if devtools was not launched from the server. If that's not the case here, we may have a race condition.

@kenzieschmoll kenzieschmoll merged commit aac0acc into flutter:master May 7, 2020
@kenzieschmoll kenzieschmoll deleted the server branch May 7, 2020 21:54
@DanTup

DanTup commented May 8, 2020

Copy link
Copy Markdown
Contributor

I think previously the catch block above would return and prevent all this code running. There are some changes in #1886 that allow connect() to log its own errors and return null though which break this. Change lgtm, though it might be cleaner to check for null further up and just return since it's likely none of this method is ever expected to run if there's no connection (this would prevent it coming back if there are further changes in this method that don't consider nulls).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants