search

supercargo / guice-persist-jooq

Guice-persist extension for using jOOQ based persistence layer
Apache License 2.0
22 stars 10 forks source link

The way of injecting a DSLContext into a DAO described in README.md is incorrect #1

Closed klimeryk closed 8 years ago

klimeryk commented 9 years ago

Hi, first of all, thanks for this module - it has helped immensly in setting up Guice with jOOQ. However, I've stumbled upon an issue with the @Transactional annotation. Namely, it wasn't working ;) Or rather, the changes were commited to the database, even though there was a rollback... After more hours of debugging and scratching my head than I'm willing to admit, I've had a relevation - the DSLContext injected into the DAO and the one in JdbcLocalTxnInterceptor was just... not the same.

The source of the problem is that injecting DSLContext directly sets it to the value bound to the current thread (for example, the one doing the initialization, injecting dependencies, etc.). However, the same thread might not handle the request and a call to a @Transactional method/class. @Transactional looks up the DSLContext in a ThreadLocal and in the (very common) case I've described above, the DSLContext injected into the DAO and the one JdbcLocalTxnInterceptor will be modifing will just simply not be the same. So, @Transactional sets autoCommit to true, but the DAO uses a different DSLContext so it doesn't matter >_<.

This subtle difference is hinted in the Guice's documentation about JPA:

Note that if you make MyService a @Singleton, then you should inject Provider<EntityManager> instead.

I've included a pull request with proposed changes to README.md that describe a proper (in my opinion) way of injecting DSLContext - in most cases the service and/or dao are singletons, and even if not, it won't hurt and will avoid introducing a nasty bug in case someone makes them singletons in the future.

tbroyer commented 9 years ago

I believe your proposed change is wrong. The README should possibly make it clear that DSLContext is thread-local, but shouldn't tell you to use a Provider. Guice-Persist is clear that a UnitOfWork is thread-local, and a unit of work with guice-persist-jooq is a DSLContext (even if JOOQ's DSLContext is otherwise threadsafe).

IIUC, there may be a bug in Guice-Persist (replicated here in guice-persist-jooq) that the Provider<Xxx> implementation in the XxxPersistService automatically calls begin() if not isWorking(). The checkState just after will thus never throw.

klimeryk commented 9 years ago

Thank you for looking into this - I was hoping that someone would verify my findings. You are most likely right, however I'm not familiar enough with guice-persist to propose a change I'd be confident to be correct. I was hoping that the author of this project (or someone else suitable) would look into this. I was basing my "fix" on an existing code, which in turn is supposed to be based on JPA's support in Guice. I've raised this issue, because I am at least confident that the current implementation is not working as it should (either because there's a bug in this project or guice-persist, as you've pointed out). Normally, I'd be happy to dig into this deeper and learn something new about Guice, however it so happens that I'm leaving the project that I need this code for, so I wouldn't be able to test my proposed patch thoroughly.

supercargo commented 9 years ago

@tbroyer and @klimeryk: just saw this (I wasn't getting github notifications for some reason) so sorry for the delay in responding. In any case, it seems that proper handling of transactions could benefit from some unit tests which may also serve as better example/docs if they happen to be tests that pass. I'll leave this PR open for now until I have a chance to verify a working approach whether it be an issue with documentation and/or Guice-Persist as @tbroyer suggests.

tbroyer commented 9 years ago

Note: we've been bitten by that behavior of get() automatically calling begin(). We didn't have PersistFilter, so each HTTP request (thread) would (implicitly) begin() but never end(), which quickly exhausted all connections in our connection pool.

IMO, guice-persist-jooq should not "pin" a connection to a thread/DSLContext. It should only do that when starting a transaction, and revert to using the connection pool outside transactions.