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

"No transaction in context" with multiple Spring contexts in one JVM #407

Closed
ilya40umov opened this issue Oct 17, 2018 · 7 comments
Closed
Assignees
Labels

Comments

@ilya40umov
Copy link

ilya40umov commented Oct 17, 2018

Exposed version: 0.10.5

There seems to be a bug in the implementation of SpringTransactionManager, which leads to "No transaction in context" in runtime.

Our set up looks approximately like this:

  • We have a few types of tests that all have a slightly different configuration of Spring context. As a result SpringTransactionManager is created multiple times within the same JVM.
  • First Spring context is loaded and creates SpringTransactionManager, which in turn gets registered through TransactionManager.registerManager and then assigned to the current thread.
  • First Spring context does not have a single transactional call, and as a result SpringTransactionManager.doCleanupAfterCompletion is never called (which would execute TransactionManager.resetCurrent(null))
  • Second Spring context is loaded and creates its own instance of SpringTransactionManager, which also gets registered via TransactionManager.registerManager, but the current thread already has an instance saved in currentThreadManager variable
  • Second Spring context is trying to run an insert in a transaction and receives:
    java.lang.IllegalStateException: No transaction in context.
        at org.jetbrains.exposed.sql.transactions.TransactionManager$Companion.current(TransactionApi.kt:70)
        at org.jetbrains.exposed.sql.QueriesKt.insert(Queries.kt:44)
  • This error only occurs once, because after it runs, SpringTransactionManager.doCleanupAfterCompletion is executed and it resets the state of thread local.

My current 'hacky' solution involved adding TransactionManager.resetCurrent(null) to initTransaction method in SpringTransactionManager, but for that I had to copy the entire SpringTransactionManager to our codebase as it's final.

@Tapac Tapac self-assigned this Oct 18, 2018
@Tapac Tapac added the bug label Oct 18, 2018
@ilya40umov
Copy link
Author

For anybody interested in a temporary "hacky" fix, I'm posting my current solution here:

@Configuration
@EnableTransactionManagement
class ExposedConfig {

    @Bean
    fun transactionManager(dataSource: DataSource) =
        FixedSpringTransactionManager(SpringTransactionManager(dataSource))

    class FixedSpringTransactionManager(
        private val stm: SpringTransactionManager
    ) : ResourceTransactionManager by stm, InitializingBean by stm, TransactionManager by stm {
        override fun getTransaction(definition: TransaectionDefinition?): TransactionStatus {
            // temporary fix for https://github.com/JetBrains/Exposed/issues/407
            TransactionManager.resetCurrent(null)
            return stm.getTransaction(definition)
        }
    }
}

@Tapac
Copy link
Contributor

Tapac commented Oct 18, 2018

@ilya40umov ,
Sorry for a late reply, but could you please replace TransactionManager.resetCurrent(null) with TransactionManager.resetCurrent(this) and check how it will work?

@bastman
Copy link

bastman commented Oct 23, 2018

Thx. The example provided has fixed it for me.
Please notice, there was a typo in the example and stm.getTransaction(definition!!) expects a non-nullable parameter. No idea if "!!" is the proper approach here.

class FixedSpringTransactionManager( private val stm: SpringTransactionManager ) : ResourceTransactionManager by stm, InitializingBean by stm, TransactionManager by stm { override fun getTransaction(definition: TransactionDefinition?): TransactionStatus { // temporary fix for https://github.com/JetBrains/Exposed/issues/407 TransactionManager.resetCurrent(this) return stm.getTransaction(definition!!) } }

I experienced the issue by executing junit5 tests
@ExtendWith(SpringExtension::class) @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.MOCK)

Tapac added a commit that referenced this issue Oct 23, 2018
…ot-devtools

#407 "No transaction in context" with multiple Spring contexts in one JVM
@Tapac
Copy link
Contributor

Tapac commented Oct 23, 2018

@bastman, @ilya40umov , could you please test your apps with snapshot dependency?

repositories {
   ...
   maven { url 'https://jitpack.io' }
}

dependencies {
   ...
   compile 'com.github.JetBrains:Exposed:-SNAPSHOT'
}

@bastman
Copy link

bastman commented Oct 25, 2018

@Tapac , I was not able to get the snapshot dependencies into my current project.

I tried ...

repositories {
mavenCentral()
jcenter()
maven { url 'https://jitpack.io' }
maven { setUrl("https://dl.bintray.com/kotlin/exposed") }
maven { setUrl("https://dl.bintray.com/sdeleuze/maven/") }
}

dependencies {
...
compile 'com.github.JetBrains:Exposed:-SNAPSHOT'
compile "org.jetbrains.exposed:spring-transaction:-SNAPSHOT"
}

gradlew assemble (output) ....

HOME:
Download https://jitpack.io/com/github/JetBrains/Exposed/-SNAPSHOT/maven-metadata.xml
Download https://jitpack.io/com/github/JetBrains/Exposed/-SNAPSHOT/Exposed--0.10.5-ge88b1f9-36.pom

Task :common:compileKotlin FAILED

FAILURE: Build failed with an exception.

  • What went wrong:
    Execution failed for task ':common:compileKotlin'.

Could not resolve all files for configuration ':common:compileClasspath'.
Could not find org.jetbrains.exposed:spring-transaction:-SNAPSHOT.
Required by:
project :common

Execution failed for task ':rest-api:compileKotlin'.

Could not resolve all files for configuration ':rest-api:compileClasspath'.
Could not find com.github.JetBrains:Exposed:-SNAPSHOT.
Required by:
project :rest-api > project :common

Any idea - how to fix it?

@Tapac
Copy link
Contributor

Tapac commented Oct 25, 2018

Looks like a jitpack can't build spring-transaction sub-module.
Could you build Exposed master and install it maven local? Or will you wait for the release to test a build?

@ilya40umov
Copy link
Author

@Tapac, thanks for the suggestion! I've just tried replacing TransactionManager.resetCurrent(null) with TransactionManager.resetCurrent(this) as you suggested and it also works (and make way more sense).
However, in general I'm not entirely sure if this is a sustainable solution for people who may have multiple spring contexts (in my case it's totally coincidental that I ran into this situation in our pipeline and in my case there is also no concurrency involved).
P.S. I'll wait for the next version of Exposed to come out and report my results then.

@Tapac Tapac closed this as completed Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants