Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Assertion to the SharedTimer Test #2357

Open
Codegass opened this issue Mar 21, 2024 · 4 comments · May be fixed by #2381
Open

Add Assertion to the SharedTimer Test #2357

Codegass opened this issue Mar 21, 2024 · 4 comments · May be fixed by #2381
Labels
Enhancement An enhancement to the driver. Lower priority than bugs.
Projects

Comments

@Codegass
Copy link
Contributor

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.

@lilgreenbird
Copy link
Member

hi @Codegass

this sounds reasonable yes please submit a PR it will go through the review process, thanks!

@lilgreenbird lilgreenbird added this to Under Investigation in MSSQL JDBC via automation Mar 21, 2024
@lilgreenbird lilgreenbird added the Enhancement An enhancement to the driver. Lower priority than bugs. label Mar 21, 2024
@lilgreenbird
Copy link
Member

closing, please go ahead and submit a PR your convenience it will go through the review process

MSSQL JDBC automation moved this from Under Investigation to Closed Issues Mar 27, 2024
@Codegass
Copy link
Contributor Author

Sure! I will submit the PR ASAP. Thanks.

@Codegass Codegass linked a pull request Apr 4, 2024 that will close this issue
@Jeffery-Wasty
Copy link
Member

Reopening since there is now a PR to address this.

@Jeffery-Wasty Jeffery-Wasty reopened this Apr 5, 2024
MSSQL JDBC automation moved this from Closed Issues to Under Investigation Apr 5, 2024
@Jeffery-Wasty Jeffery-Wasty moved this from Under Investigation to Under Peer Review in MSSQL JDBC Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement An enhancement to the driver. Lower priority than bugs.
Projects
MSSQL JDBC
  
Under Peer Review
Development

Successfully merging a pull request may close this issue.

3 participants