Skip to content

Stop Http2ClientTest from calling live nghttp2.org - #3503

Open
arimu1 wants to merge 1 commit into
OpenFeign:14.xfrom
arimu1:fix/3483-http2client-mockwebserver
Open

Stop Http2ClientTest from calling live nghttp2.org#3503
arimu1 wants to merge 1 commit into
OpenFeign:14.xfrom
arimu1:fix/3483-http2client-mockwebserver

Conversation

@arimu1

@arimu1 arimu1 commented Jul 31, 2026

Copy link
Copy Markdown

What

Http2ClientTest on 14.x had four tests that hit live https://nghttp2.org/httpbin/ after a DNS-only reachability check. When DNS resolves but the HTTP service is down or slow, those tests fail or hang the whole build.

How

  • patch / noResponseBodyForPatch / deleteWithRequestBody now use the existing MockWebServer fixture.
  • getWithRequestBody uses a small local ServerSocket server (MockWebServer rejects GET requests that carry a body).
  • Removed the HTTPBIN_REACHABLE DNS gate and the JSONAssert dependency from these tests.
  • Approach matches what already landed on master.

Testing

./mvnw -Dtoolchain.skip=true -pl java11 -am test -Dtest=Http2ClientTest -Dsurefire.failIfNoSpecifiedTests=false

Result: Tests run: 33, Failures: 0, Errors: 0, Skipped: 0

Fixes #3483

Replace DNS-gated live httpbin requests with MockWebServer for
PATCH/DELETE and a local socket server for GET-with-body, matching the
master approach so offline/broken external hosts cannot hang CI.

Fixes OpenFeign#3483

@kdelay kdelay 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.

Ran this on the PR head (3408015) with JDK 26.0.1 on macOS arm64:

./mvnw -Dtoolchain.skip=true -pl java11 -am test -Dtest=Http2ClientTest -Dsurefire.failIfNoSpecifiedTests=false
Tests run: 33, Failures: 0, Errors: 0, Skipped: 0

The result you reported reproduces, and the three MockWebServer conversions (patch, noResponseBodyForPatch, deleteWithRequestBody) read correctly to me. One point on getWithRequestBody I think is worth settling before this lands.

The local socket server turns a future regression into a ~108s failure with the wrong message.

The stated goal is that CI no longer hangs or fails on an external host. The three MockWebServer tests get that. getWithRequestBody does not, because writing the response is on the success path only.

The read loop exits on contains("some request body") or EOF. If the request body ever stops arriving, which is exactly the regression this test exists to catch, the loop blocks until setSoTimeout(5000) fires, the thread dies with UncheckedIOException, and no response is ever written.

I measured it by changing only the sentinel string to one that never appears and leaving the rest of your branch untouched:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 107.7 s
[ERROR] Http2ClientTest.getWithRequestBody:215 » Retryable HTTP connect timed out executing GET http://localhost:52262/anything

The real cause appears only in system-err of the surefire report, detached from the failure:

Exception in thread "Thread-1" java.io.UncheckedIOException: java.net.SocketTimeoutException: Read timed out

So a missing request body is reported as a connect timeout, 108 seconds later. On the timing: the first connection is accepted and then dropped when the server thread dies, and subsequent retry attempts land in a backlog of 1 that nobody accepts again. I did not instrument the individual retries, so treat that mechanism as my reading of the numbers rather than something I measured directly.

Two adjustments, both already idiomatic in this file:

  1. timeoutTest just above uses .retryer(Retryer.NEVER_RETRY) with an explicit Request.Options. The same here bounds a failure to one short attempt instead of the retried default.
  2. Run the server body on an ExecutorService or CompletableFuture and resolve it with a timeout before asserting, so a server-side IOException surfaces as the test failure rather than a stray stderr trace. That also covers serverThread.join(5000), whose return value is currently ignored: if the join times out the test still proceeds and asserts on a possibly-null receivedRequest, which surfaces as "Expecting actual not to be null" instead of the underlying I/O error.

For the record, I only exercised this branch; I did not compare against the master-side approach you mention.

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.

2 participants