Skip to content

core: Handle ProxySelector IllegalArgumentException - #13053

Merged
AgraVator merged 2 commits into
grpc:masterfrom
kkmurthyt21:core-13052-proxy-selector
Oct 14, 2026
Merged

AgraVator merged 2 commits into
grpc:masterfrom
kkmurthyt21:core-13052-proxy-selector

Conversation

@kkmurthyt21

@kkmurthyt21 kkmurthyt21 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #13052.

Android’s DefaultProxySelector.select() can throw IllegalArgumentException when a configured proxy port is invalid, causing an unchecked exception to escape the DNS resolver task.

This change wraps that exception in an IOException with the message "Failed when selecting a proxy", preserving the original cause. The resolver’s existing error handling reports UNAVAILABLE.

Testing

  1. Updated ProxyDetectorImplTest to verify the message and use assertSame for the original cause.
  2. Added a DnsNameResolverTest regression test exercising the proxy-selection failure through the real proxy detector and resolver executor. It verifies UNAVAILABLE, cause preservation, and no direct DNS lookup.
  3. All 70 tests in the two affected classes passed locally. Both style checks passed.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: kkmurthyt21 / name: Krishna Kandi (bc01614)

@AgraVator

Copy link
Copy Markdown
Contributor

Thanks for picking this up. Approach is right, a few things need changing.

The test doesn't cover the actual bug. ProxyDetectorImpl throwing IOException was already
handled before this PR the crash is the unchecked exception escaping DnsNameResolver$Resolve.run()
onto the executor thread. As written the test passes on a build that still crashes. There's a
pattern to copy in DnsNameResolverTest around L519 (thenThrow(new IOException())); the same with
an IllegalArgumentException, asserting the listener gets a result, fails on master today.

Related: select() isn't the only unguarded call on that path. A few lines down
requestPasswordAuthentication() lands in java.net.Authenticator, which apps install via
setDefault() same thread, same crash. So this also wants a catch around detectProxy() in
Resolve.run() converting unchecked to IOException, and I'd widen the catch here to
RuntimeException rather than just IAE.

Also worth knowing: the message you're building never reaches the user, because DnsNameResolver
overrides the description with "Unable to resolve host " + host. I've argued for falling back to
no-proxy on the issue instead probably worth waiting for that to settle before reworking, no point
doing it twice.

IOException exception =
assertThrows(IOException.class, () -> proxyDetector.proxyFor(destination));

assertEquals(cause, exception.getCause());

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.

Nit: assertSame for the cause.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a close look. This is helpful. I agree that a resolver level test would better cover the actual failure path, and I will change the cause check to assertSame.

Before I rework the implementation, could one of the maintainers confirm the preferred behavior? Should this fall back to a direct connection, or return UNAVAILABLE? Also, should the broader RuntimeException handling be placed around detectProxy() in DnsNameResolver?

Once that is confirmed, I will update the implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @ejona86 , I wanted to follow up on the behavior decision for this PR. Should a RuntimeException from detectProxy() fall back to a direct connection, or should it return UNAVAILABLE? Once the preferred behavior is confirmed, I can add the DnsNameResolver regression test and update the exception handling accordingly. Thank you.

Comment thread core/src/main/java/io/grpc/internal/ProxyDetectorImpl.java Outdated
@ejona86
ejona86 requested a review from AgraVator October 12, 2026 16:06
@AgraVator AgraVator added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Oct 14, 2026
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Oct 14, 2026
@AgraVator
AgraVator merged commit 6f5fcce into grpc:master Oct 14, 2026
15 of 18 checks passed
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.

Android: ProxyDetectorImpl crashes when DefaultProxySelector contains an invalid proxy port

4 participants