Fix flaky vertx http2NoConnectionLeak test by using h2c prior knowledge - #3385
Merged
Conversation
Signed-off-by: Marvin Froeder <velo.br@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feign.vertx.ConnectionsLeakTests.http2NoConnectionLeakhas been flaky on CI (e.g. failed on #3384), assertingExpected size: 1 but was: 2open server connections.Root cause
The test fires 100 concurrent requests over an HTTP/2 pool capped at one connection (
PoolOptions.setHttp2MaxSize(1)) and asserts the server saw exactly one connection.By default vertx negotiates cleartext HTTP/2 via an HTTP/1.1 Upgrade (
HttpClientOptions.DEFAULT_HTTP2_CLEAR_TEXT_UPGRADE = true). During that handshake the connection is still HTTP/1.1, so the pool applieshttp1MaxSize(default 5) instead ofhttp2MaxSize(1). Under CI load the upgrade window is wide enough that a second connection is opened before the first finishes upgrading to HTTP/2, so the server records 2 connections. (DEFAULT_MAX_CONCURRENT_STREAMSis effectively unlimited, so stream multiplexing is not the issue.) It surfaced after the recent vertx 5.1.0 bump because the fullpr-buildsuite only runs on PRs.Fix
Connect with h2c prior knowledge (
setHttp2ClearTextUpgrade(false)): the connection is HTTP/2 from the first byte, sohttp2MaxSize(1)deterministically caps it at one connection and the 100 requests multiplex over it. Applied to both thefeign-vertxtest (run against vertx 5.x viafeign-vertx5-test) and thefeign-vertx4-testcopy.Validation
ConnectionsLeakTests(both HTTP/1.1 and HTTP/2) passes locally under vertx 5.1.0 (5/5 runs) and vertx 4.5.27.