Re: [hibernate-dev] [renamed] multiple invocations on same transaction, separate thread is not working unless is changed to false...

Steve, we had a conversation about this on IRC, I think we came to a ​ hopefully​ nice ​ ​solution . [15:18] <sannegrinovero> andrigtmiller: I still think you're locking out the TXM, not making it possible to legitimately timeout queries.. [15:19] <sannegrinovero> (but I don't have the full picture.. not really my area) [15:19] <smarlow> that is a transaction manager concern but they already handled calling other synchronization call backs. currently, Arjuna holds a lock on the collection of synchronizations at that time, so there should be no conflict [15:20] <smarlow> I asked about your concern yesterday to make sure we wouldn't deadlock [15:20] <sannegrinovero> I wasn't actually thinking about deadlock, I just don't see what you're aiming at with the single lock on the Session [15:22] <smarlow> to avoid calling Hibernate session.clear from a background thread while the application thread is also doing a Hibernate session invocation. which can lead to random exceptions [15:22] <smarlow> I'm aiming at solving the concurrency concern ^ [15:22] <sannegrinovero> But I guess you'd need to talk with sebersole, this lock thing you're proposing comes out of the blue for me :) [15:22] <sannegrinovero> yes I get that [15:23] <sannegrinovero> but you should allow the session.clear() to proceed, not lock it out [15:23] <smarlow> sure, I'm just trying to talk to who ever listens and have feedback. Before I get anyway near code changes, Sebersole needs to be on board (he isn't yet) [15:24] <sannegrinovero> I'm neither :) willing to discuss for sake of interest, but can't replace sebersole on this I think :) [15:24] <smarlow> I posted on as7 dev mailing list a few years ago and the only feedback that I got was that only Hibernate was not handling concurrency but there could be others [15:25] <sannegrinovero> what is the effect you expect to have users see when the background thread kills the current session? [15:25] <smarlow> I cross posted as well [15:25] <smarlow> users shouldn't get NPE errors or unexpected exceptions [15:26] <sannegrinovero> sure, but what kind of effect would you propose? [15:26] <sannegrinovero> I guess a different exception with a better error message? [15:26] <smarlow> my goal is ensuring that only one thread is invoking the Hibernate session at a time [15:27] <sannegrinovero> No that's not true, as you're saying that you want to allow the TXM to rollback the transaction [15:27] <smarlow> well, want is a strong word :) [15:28] <smarlow> IBM/Oracle/JBoss all do that [15:28] <sannegrinovero> and that's an implementation detail what you just explained :) [15:28] <sannegrinovero> what I'm asking is what you expect the user to experience when this needs to happen [15:29] <smarlow> when the transaction is rolled back, depending on where the application code is in the Hibernate session invocation, a JDBC error might occur or something related to that. The goal is to avoid mutating the Hibernate session from two different threads concurrently [15:30] <sannegrinovero> you're not answering my question, that's an implementation detail :) [15:30] <smarlow> if we can avoid violating concurrency of the hibernate session, I think we will be more robust. [15:31] <sannegrinovero> The goal of the TXM timeout, is to kill stuff which is taking too long.. not allowing it to finish in a safe lock. andrigtmiller am I understanding the basics correctly? [15:31] <smarlow> currently, what the user will experience is poor. The scope of turning that into a more pleasant experience is a good question but involves many moving and separate parts [15:32] <sannegrinovero> so you actually _need_ to allow concurrent access, to kill and rollback the operations which are being done [15:32] <smarlow> the goal of TXM timeout handling in JBoss/Oracle/IBM is to also handle deadlocks that might not otherwise be recovered from [15:32] <smarlow> the concurrent access happens at the resource level [15:33] <smarlow> from the background, with components that handle concurrency. Hibernate doesn't [15:33] <sannegrinovero> that's just one part of the things [15:33] <sannegrinovero> and we can definitely have Hibernate handle some concurrent events [15:33] <smarlow> Hibernate sessions are not supposed to handle concurrency, so its a design flaw [15:33] <sannegrinovero> the TXM shouldn't invoke clear() (which is public API) but invoke a specific method which could provide the needed semantics [15:33] <smarlow> its not some events, its many events [15:34] <sannegrinovero> why do you say it's a design flaw? it's not, for the "normal" public API usage [15:34] <smarlow> the TXM, calls the Hibernate Synchronization.afterCompletion(int) and Hibernate detaches entities [15:34] <smarlow> and Hibernate does other cleaning up as well [15:35] <smarlow> its a design flaw as its not handled in our system [15:35] <sannegrinovero> I don't feel I'm making progress in the conversation if you keep repeating the implementation details, we can fix that as we please [15:35] <sannegrinovero> but you have to explain the expected user experience [15:36] <smarlow> you mean if we didn't have any constraints of any of the existing specs that we implement? [15:36] <sannegrinovero> yes, just explain what you think it should do please, that would help me understand, and I think I can propose you something more concrete [15:37] <smarlow> if we ignore XA, JTA + JPA, I can answer the question better and will attempt that [15:37] <sannegrinovero> the problem is the user sees an NPE right? [15:38] <smarlow> could be an NPE or any unexpected exception that comes from using a thread-unsafe component from multiple threads [15:38] <sannegrinovero> so the basic question is what do you think the user should "see".. I guess another exception, say HibernateRolledBackException("The Transaction Manager decided we run out of time") [15:38] <sannegrinovero> sure. would you like the above proposal ^ [15:39] <sannegrinovero> Because implementing that is easy :) [15:41] <sannegrinovero> ? If you're busy I'm happy to talk later, I am 3h late with my lunch [15:41] <smarlow> in a perfect world, 1) the user registers an application event listener that tells that the transaction timed out. 2) whether the application thread catches a signal that the transaction is about to be rolled back. 3) The application thread then catches a signal that the transaction was rolled back [15:41] <sannegrinovero> that doesn't explain what you expect the client code to experience. [15:41] <smarlow> 4) the application thread then catches a signal that Synchronization call backs are going to happen to clean up after the roll back [15:42] <sannegrinovero> say the user was incoking a "Query.list()" .. what do we return? [15:42] <smarlow> what could they expect? They could be in the middle of code that doesn't use the Hibernate session or could be in code that does [15:43] <sannegrinovero> Exactly my point, so we'd return an exception like I proposed above right? [15:43] <smarlow> if you say so but no one knows how to do that [15:43] <smarlow> I mean, I'm not sure what returns that [15:44] <smarlow> we had tried doing what you suggest but it was too performance expensive and covered too few cases [15:44] <smarlow> but sure, throwing an EJB rolledback exception is ideal [15:45] <sannegrinovero> you can do it in an efficient way, but yes I agree you'd have to patch several areas of code. [15:45] <sannegrinovero> I'd do it incrementally though, start with the Session and Query APIs, see if people like it [15:45] <smarlow> our previous attempt that failed, added a pre-check at the front of every Hibernate session call, tail end and middle [15:46] <smarlow> but that didn't work because it didn't handle remote transactions or distributed transactions [15:46] <sannegrinovero> the EntityManager implementation has some kind of "exception translator" [15:46] <smarlow> and performance suffered [15:46] <sannegrinovero> all you need to do is catch exceptions, and re-throw the better explanation [15:46] <smarlow> so, we have less of the checking today and still don't handle remote transactions and distributed transactions [15:47] <sannegrinovero> the TXM just needs to flag a volatile field in the Session to inject the error it wants us to tell [15:47] <sannegrinovero> so on the optimal path you just have a volatile field read operation, which is a zero cost essentially [15:47] <sannegrinovero> (optimal path I mean the case in which there are no errors) [15:48] <smarlow> then we need to poll for that flag in the start/middle/end (or so) if every Hibernate method [15:48] <sannegrinovero> no [15:48] <smarlow> or maybe in the finally clause of every method [15:48] <sannegrinovero> you just need to catch exceptions [15:48] <sannegrinovero> and we happen to already do that, because you need to translate all Hibernate specific exceptions to JPA specific ones [15:49] <sannegrinovero> so it's actually a very simple patch with zero overhead I think [15:49] <smarlow> at worse, we catch exceptions (including NPE) and notice the flag and then eat the cause exception? [15:49] <sannegrinovero> +1 [15:49] <smarlow> if we show the cause, its confusing [15:49] <sannegrinovero> you don't need the finally method either though [15:49] <smarlow> if we eat it, its confusing [15:49] <sannegrinovero> right, don't show the cause. [15:49] <smarlow> I hate when exceptions are eaten [15:50] <sannegrinovero> well.. the TXM should provide you a sensible error message, like "aborted because of ... ", and we take that as explanation. [15:50] <smarlow> but only if that flag is set [15:50] <sannegrinovero> of course! [15:50] <smarlow> sounds like a worthwhile idea, thanks [15:51] <sannegrinovero> np, I'm very happy if it works :) [15:51] <sannegrinovero> the exception translation is already done somewhere in the EM project [15:51] <sannegrinovero> I don't remember the details, but I'm sure you're more familiar with it [15:51] <sannegrinovero> just make sure you look that up [15:52] <sannegrinovero> it catches all exceptions from Hibernate to wrap them in something else, to satisfy the spec requirements -- Sanne