transactions are rolled back if thread is killed during transaction #382
Conversation
|
I think attempting to be well behaved in the presence of |
ghost
reopened this
|
Sorry, should have given context, this isn't to deal with the issue Headius raises. http://coderrr.wordpress.com/2011/05/03/beware-of-threadkill-or-your-activerecord-transactions-are-in-danger-of-being-partially-committed/ |
|
I realise that, however my point remains, it's essentially impossible to have deterministic code in the presence of Thread#kill, your code should never call it. Why were you calling it? |
|
Did you read the post? It pretty much explains the issue exactly. tldr is all threads are Thread#killed when a process terminates. |
|
Indeed I did read the post, thanks for asking :P What I didn't see is why you didn't register a TERM handler? My objection remains all the same, #kill and #raise can arise basically anywhere and I'm not sold that having a single potential case addressed with code that only runs on a subset of our supported runtimes is justified here given the literally endless number of cases which can be result from the methods in question. |
|
K so firstly I did mention that a signal handler is what you should do and mitigates the issue. Of course if something goes wrong in your signal handler you end up getting Thread#killed again. Secondly, this isn't a solution for Thread#raise at all, only Thread#kill. Thirdly and most importantly, this makes sure you will NEVER get a partially committed transaction due to a Thread#kill. Doesn't matter where the interpreter is when it's called. Of course, this is only true on 1.8 and Rubinius unless the others also change their thread status behavior (which I think they probably should). |
|
My next suggestion was going to be to refactor the change so that instead of changing here it's in commit_db_transction (it could simply raise if the thread is aborting). However that would prevent you from deliberately commiting transactions in ensure blocks when the thread is being killed.... In fact doesn't your code here prevent people from committing in ensure blocks? I can definitely see your point, but a partial fix for a subset of interpreters isn't really good enough. I'll see if I can ping the jruby guys to get their thoughts on the change and whether they intend changing their code's behaviour |
|
Yea it'd be great if you could try to get 1.9 and JRuby to implement matching Thread.current.status behavior. My code only prevents transactions from being committed when the Thread is being killed. This is how it should be because if the Thread is being killed you have no way of knowing whether the transaction was completed or not (aside from the local var / flow control stuff I talked about in the post). The only issue I see with raising in commit_db_transaction if the Thread is being killed is that (unless you catch that exception) then you won't explicitly call rollback. But maybe that isn't an issue. |
|
This is a reasonable concern, but I think the platform-specific fix isn't going to fly since everyone's moving to 1.9.x where it doesn't even work. Is there a better way? How about the local variable approach in your blog post? |
|
@jeremy, not that I could figure out when I was researching this. The local variable approach will roll back any transaction that uses the flow control statements I mention in the post. I think the only hope is to get 1.9 and jruby to set Thread#status to 'aborting' consistently. |
|
@mhfs No responses for 2 months; close it. |
|
@jeremyf I don't have permission to close it... |
|
I can close, but I guess we should not. I think that issue is still there either with Ruby 1.9. I'm asking to the core team. |
|
Closing as we only support 1.9 now and the proposed changes here don't work there. |
... as opposed to being partially committed. Only fixes behavior in MRI 1.8 and Rubinius. JRuby and 1.9 will still work the same since their Thread.current.status won't be 'aborting'