-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Add workaround for https://issuetracker.google.com/issues/63649622 #3624
Add workaround for https://issuetracker.google.com/issues/63649622 #3624
Conversation
} catch (ClassCastException e) { | ||
// On android 8.0, socket.connect throws a ClassCastException due to a bug | ||
if (Build.VERSION.SDK_INT == 26) { | ||
log(WARN, "Swallowed ClassCastException, see https://issuetracker.google.com/issues/63649622", 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.
Without seeing this exact Android code, it seems hard to review. But I don't think this is correct based on previous versions. It seems like the CCE is thrown while processing a root IOException. I'm basing this on connectDetail always going into an Exception.
At minimum swallowing only seems correct if the connect is successful, and then the CCE is thrown during routine logging. The Google bug mentions this likely being related to sockets cancelled on another thread. Ultimately it may not matter as the next IO operation is likely to fail, but it would be better if we get the state right at the end of this method.
I suspect the correct fix is rethrowing as an IOException.
@JakeWharton do you know anyone who could check the exact state after this exception?
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.
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.
@yschimke and for reference, there is the fixed version of the Android code, which did not make it to the initial release: https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/libcore/io/IoBridge.java
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.
tl;dr: rethrowing the exception as IOException seems about right.
Nice. Thank you! |
…quare#3624) * Add workaround for https://issuetracker.google.com/issues/63649622 This fixes square#3438 * Rethrowing the Android O bug CCE as IOException
This fixes #3438