microsoft/mssql-jdbc

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.