-
Notifications
You must be signed in to change notification settings - Fork 616
Don't crash SDK is socket.close() fails #2216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (af627623) is created by Prow via merging commits: 141830d eb4f8f8. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (af627623) is created by Prow via merging commits: 141830d eb4f8f8. |
@@ -261,8 +261,8 @@ private synchronized void closeSocket() { | |||
if (socket != null) { | |||
try { | |||
socket.close(); | |||
} catch (IOException e) { | |||
throw new RuntimeException(e); | |||
} catch (Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catching Throwable
seems a little bit aggressive as it will also catch java.lang.Error
s which, according to its javadoc, "a reasonable application should not try to catch". Would it be sufficient to only catch Exception
for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I only mean to catch IOException and NPEs here.
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
* RTDB Query get method must be public (#2231) * Don't crash SDK is socket.close() fails (#2216) Co-authored-by: Jan Wyszynski <[email protected]> Co-authored-by: Sebastian Schmidt <[email protected]>
On some Pixel devices,
socket.close()
throws an NPE (might be related to google/conscrypt#331).Fixes b/164528104