Add Assertion to the `SharedTimer` Test
Codegass opened this issue · 4 comments
Is your feature request related to a problem? If so, please give a short summary of the problem and how the feature would resolve it
When I am walking through the unit test of the mssql-jdbc, I noticed that currently the SharedTimerTest
does not contain any assertion statement to verify the behavior and state of the SharedTimer
after execution of its getTimer()
method in a multithreaded environment. The test ensures that references are added and removed, but does not assert the expected state of the SharedTimer (e.g., it is stopped and cleaned up properly after all references are removed). This could potentially mask issues with how SharedTimer
manages its lifecycle and resources.
Current Implementation:
@Test
void getTimer() throws InterruptedException, ExecutionException, TimeoutException {
final int iterations = 500;
ExecutorService executor = Executors.newFixedThreadPool(2);
try {
ArrayList<CompletableFuture<?>> futures = new ArrayList<>(iterations);
for (int i = 0; i < iterations; i++) {
futures.add(CompletableFuture.runAsync(() -> SharedTimer.getTimer().removeRef(), executor));
}
CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(2, TimeUnit.MINUTES);
} finally {
executor.shutdown();
}
}
Describe the preferred solution
I'd like to propose a minor improvement by adding a simple assertion at the end of the getTimer test method to verify that the SharedTimer has indeed been stopped and its resources have been cleaned up after all references to it have been removed. This could be accomplished by asserting that SharedTimer.isRunning() returns false, indicating that the internal ScheduledThreadPoolExecutor has been shut down and the SharedTimer instance has been cleaned up. Here is the suggested code snippet for this improvement:
@Test
void getTimer() throws InterruptedException, ExecutionException, TimeoutException {
final int iterations = 500;
ExecutorService executor = Executors.newFixedThreadPool(2);
try {
ArrayList<CompletableFuture<?>> futures = new ArrayList<>(iterations);
for (int i = 0; i < iterations; i++) {
futures.add(CompletableFuture.runAsync(() -> SharedTimer.getTimer().removeRef(), executor));
}
CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(2, TimeUnit.MINUTES);
} finally {
executor.shutdown();
if (!executor.awaitTermination(800, TimeUnit.MILLISECONDS)) {
executor.shutdownNow();
}
}
assertFalse(SharedTimer.isRunning(), "SharedTimer should be stopped after all references are removed.");
}
Describe alternatives you've considered
An alternative would be to incorporate more detailed logging within the SharedTimer class regarding its state transitions (e.g., when references are added or removed, and when it starts or stops). While this would provide more insight during test execution, it would not replace the need for explicit assertions to verify the correct behavior programmatically.
Additional context
Reference Documentations/Specifications
This suggestion does not directly relate to any specific JDBC Specifications.
Reference Implementation
If this suggestion is helpful, I am more than happy to submit a Pull Request to implement the refactoring.
hi @Codegass
this sounds reasonable yes please submit a PR it will go through the review process, thanks!
closing, please go ahead and submit a PR your convenience it will go through the review process
Sure! I will submit the PR ASAP. Thanks.
Reopening since there is now a PR to address this.