Add regression tests for the net client close hook registration - #6314
Open
jnbdz wants to merge 1 commit into
Open
Add regression tests for the net client close hook registration#6314jnbdz wants to merge 1 commit into
jnbdz wants to merge 1 commit into
Conversation
Motivation: Closing a net client did not unregister it from its owner close future, so every client created with createNetClient() stayed reachable for the life of the Vert.x instance. This was fixed by 6a0a037, which made the resource hook remove itself from its owner when shut down, but nothing covers the net client case. Changes: Add two tests mirroring testFinalizeHttpClientRegisteredWithCloseHook, one for a client created in a verticle context and one for a client created with the Vert.x instance as owner. Both fail before 6a0a037.
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
1 participant
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.
Motivation
#6311 reports that a client created with
createNetClient()is never released, even afterclose()completes, so each call leaves oneNetClientImplreachable for the life of the Vert.x instance.The leak is real but is already fixed on master. Bisecting with the tests in this PR:
expected null, but was:<io.vertx.core.net.impl.tcp.NetClientImpl@...>ResourceHook.shutdownnow callsowner.remove(this)before delegating to the resource, so shutting the client down — explicitly or through the cleaner — unregisters the close hook and the client becomes collectable. That generalised the HTTP client fix from abab114, whoseClientCloseableResourcecarried the note that it "should somehow get unified and reused".Nothing covers the net client case, so this adds the missing tests rather than a fix.
Change
Two tests mirroring
testFinalizeHttpClientRegisteredWithCloseHook:testFinalizeNetClientRegisteredWithCloseHook— client created inside a verticle, so the owner is the context close future; it also asserts the client does not resurface after undeploy.testFinalizeNetClientRegisteredWithVertxCloseHook— client created with the Vert.x instance as owner, matching the reproducer in the issue.Both were verified to fail at d9e1080 and pass from 6a0a037 onwards, so they lock in the behaviour rather than merely passing today.
Test
Full
VertxTest: 21 tests, 0 failures.Note
The 5.1 branch does not contain 6a0a037 and still leaks —
testFinalizeNetClientRegisteredWithVertxCloseHookfails there at 6b710f6, which matches the 5.1.6 report. I left that out of this PR since the backport is your call; happy to open one against 5.1 if you want it.