fix(tests): prevent HashedWheelTimer leak in integration tests#99
Open
dkasimovskiy wants to merge 4 commits into
Open
fix(tests): prevent HashedWheelTimer leak in integration tests#99dkasimovskiy wants to merge 4 commits into
dkasimovskiy wants to merge 4 commits into
Conversation
The integration test base classes in tarantool-core, tarantool-schema,
tarantool-jackson-mapping, and tarantool-client created a static
HashedWheelTimer that was never released, producing a Netty leak warning
('LEAK: HashedWheelTimer.release() was not called before it's
garbage-collected') for every test class on every CI run.
- Promote timerService and factory to static (shared across tests in a
class for efficiency) and register a JVM shutdown hook that calls
timerService.stop() and cancels any pending timeouts at JVM exit. A
shutdown hook is used instead of @afterall because the static fields in
BaseTest are per-declaring-class and would otherwise be stopped after
the first subclass, breaking subsequent test classes.
- IProtoClientTest.testTimeoutCancel was reassigning the shared static
timer; refactored to use a local HashedWheelTimer and ConnectionFactory
so the cancellation behavior is still exercised without affecting other
tests.
- IProtoClientWatchersTest.checkTTVersion was throwing NPE when the
TARANTOOL_VERSION environment variable was not set (the case for local
runs; CI sets it). Fall back to the actual container version when the
env var is unset.
…sions Extract the four identical inline shutdown hooks (one per test class) into a single TimerShutdownHook helper. Two copies of the helper live in tarantool-core and tarantool-schema test sources; client and jackson reuse the core copy via a new test-jar dependency on tarantool-core. schema does not depend on core so the helper is duplicated there rather than introducing a new cross-module dependency. Also revert a dead `version == null` ternary branch in IProtoClientWatchersTest.checkTTVersion (the null branch returned a value that never changed the assertion outcome) and rename testTimeoutCancel to testLocalTimerHasNoPending while inlining the redundant localFactory variable, so the test name describes what it actually does after the static-timer refactor in f2d5c4d. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous commit added <type>test-jar</type> dependencies on tarantool-core in client and jackson-mapping pom.xml. The maven-jar-plugin only produces the test-jar artifact during the `package` phase, which `mvn test` (used by CI's unit profile) does not reach. Downstream modules therefore fail with "Could not find artifact ... in central-portal-snapshots". Replace the cross-module test-jar deps with a copy of TimerShutdownHook in each of the four modules' own test sources (the same pattern already used for tarantool-schema in the previous commit). This matches the plan's "Option A" recommendation that was originally only applied to schema. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dkasimovskiy
changed the title
dkasimovskiy
changed the title
Alex-pvl
requested changes
Jun 19, 2026
| private TimerShutdownHook() {} | ||
|
|
||
| public static void register(Supplier<Timer> timerRef, String name) { | ||
| Runtime.getRuntime() |
Contributor
There was a problem hiding this comment.
Suggested change
| Runtime.getRuntime() | |
| Runtime.getRuntime().addShutdownHook(new Thread(timer::stop, name)); |
stop() не возвращает null, а cancel() не работает на остановленном таймере
| public final class TimerShutdownHook { | ||
| private TimerShutdownHook() {} | ||
|
|
||
| public static void register(Supplier<Timer> timerRef, String name) { |
Contributor
There was a problem hiding this comment.
Supplier избыточен
Suggested change
| public static void register(Supplier<Timer> timerRef, String name) { | |
| public static void register(Timer timer, String name) { |
…direction Per PR #99 review feedback: Timer.stop() already cancels all pending tasks (per Netty 4.2 javadoc), so the explicit pending.forEach(cancel) pass in the shutdown hook body is dead work. After stop() returns the worker thread is dead, so cancel() on the returned handles is a no-op, and stop() never returns null. Also drop the Supplier<Timer> wrapper at every call site — the timer reference is a static field, stable for the JVM lifetime. Body collapses to one line per module: Runtime.getRuntime().addShutdownHook(new Thread(timer::stop, name)); Net diff: -56 LOC across 8 files. box-integration profile passes (Tarantool 2.11.8). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.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
2 participants
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.
What this PR does
Fixes a
HashedWheelTimerleak that integration tests caused on every CI run, then deduplicates the per-class shutdown hook that stops the leaked timer.Background
Integration test base classes in
tarantool-core,tarantool-schema,tarantool-jackson-mapping, andtarantool-clientcreated a staticHashedWheelTimerthat was never released, producing a NettyLEAK: HashedWheelTimer.release() was not called before it's garbage-collectedwarning for every test class on every CI run.Changes
Promote
timerServiceandfactoryto static and register a JVM shutdown hook per test base class that callstimerService.stop()and cancels any pending timeouts at JVM exit. A shutdown hook is used instead of@AfterAllbecause the static fields live on the per-declaring-classBaseTest, so@AfterAllin one subclass would stop the timer before later subclasses run.Refactor
IProtoClientTest.testTimeoutCancelto use a localHashedWheelTimerandConnectionFactoryso the cancellation behaviour is still exercised without mutating the shared static timer that other tests now rely on.In
IProtoClientWatchersTest.checkTTVersion, fall back to the actual container version when theTARANTOOL_VERSIONenvironment variable is unset, instead of throwingNullPointerException(CI sets the env var; local runs do not).Extract the four identical inline shutdown-hook blocks into a single
TimerShutdownHookhelper. A copy of the helper lives in each of the four modules' own test sources — no cross-module test-jar dependency.Revert the dead
version == nullternary branch introduced alongside the env-var fallback (the null branch returned a value identical to the non-null branch and never changed the assertion outcome).Rename
testTimeoutCanceltotestLocalTimerHasNoPendingand inline the redundantlocalFactorylocal, so the test name describes what it actually does after the static-timer refactor.Diff shape
No production code touched. Test sources only.
I haven't forgotten about:
Related issues:
Refines #98