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

feat: EXPOSED-225 Support transaction timeout in SpringTransactionManager #1897

Conversation

FullOfOrange
Copy link
Contributor

@FullOfOrange FullOfOrange commented Nov 24, 2023

related issue #1671

Simply changed the Spring Transaction's timeout to apply. Change the implementation to use the timeout implemented in the previous PR.

The test code for SpringTransactionManager is not database-specific, so I think we need to implement a module like withDB in the exposed-test module to test the actual behaviour. So I'm going to change the existing SpringTransactionManagerTest to be more extensible, at which point I'll add a real DB test for queryTimeout.

I wonder if the tests included in this PR are sufficient for this feature to be deployed.

Comment on lines +110 to +112
if (definition.timeout != TransactionDefinition.TIMEOUT_DEFAULT) {
queryTimeout = definition.timeout
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultTimeout has a value of -1. If it is not this value, the user has set a new value, so I added this condition.

Comment on lines +387 to +388
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spring-transaction tests run on H2 only, so an H2 hack will be needed.

I'm not the biggest fan of this, but if it's just one test and it gives us peace of mind for this feature, an option is using a recursive query to simulate a long-running query:

tm.executeAssert(initializeConnection = true, timeout = 1) {
    try {
        TransactionManager.current().exec(
           """
               WITH RECURSIVE T(N) AS (
               SELECT 1
               UNION ALL
               SELECT N+1 FROM T WHERE N<10000000
               )
               SELECT * FROM T;
            """.trimIndent(),
            explicitStatementType = StatementType.SELECT
        )
        fail("Should have thrown a timeout exception")
    } catch (cause: ExposedSQLException) {
        assertTrue { cause.cause is SQLTimeoutException }
    }
}

I also tried this in the ExposedTransactionManagerTest.kt file using the annotation @Transactional(timeout = 1) and it worked.
Do you think this is solid enough or do you think we need to try something in the spring boot tests?

Copy link
Contributor Author

@FullOfOrange FullOfOrange Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since timeout has already been tested in Exposed statements and in Transactions, I thought it would be okay to check that the SpringTransactionManager using Transactions sets the value in Transactions correctly.

However, since I know the implementation of this code, I'm thinking this way, and obviously I need to test the actual DB, I think the timeout using recursion is meaningful enough, so I'll add the test code you suggested. Thank you.

@bog-walk bog-walk requested a review from e5l November 26, 2023 00:39
@bog-walk bog-walk merged commit af6728e into JetBrains:main Nov 27, 2023
3 checks passed
@FullOfOrange FullOfOrange deleted the fulloforange/spring-transaction-manager-timeout branch November 27, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction timeout does not work in SpringTransactionManager
3 participants