transactions are rolled back if thread is killed during transaction #382

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@ghost

ghost commented May 4, 2011

... 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'

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz May 9, 2011

Member

Thread#kill and Thread#raise are basically impossible to use in a safe and reliable manner because the exceptions can come from anywhere and be of any type, Charles from jruby had a great write up about it several years ago. For example your particular patch won't help if the exception comes inside the commit_db_transaction method itself.

I think attempting to be well behaved in the presence of Thread#kill is going to be too much effort to be worthwhile.

Member

NZKoz commented May 9, 2011

Thread#kill and Thread#raise are basically impossible to use in a safe and reliable manner because the exceptions can come from anywhere and be of any type, Charles from jruby had a great write up about it several years ago. For example your particular patch won't help if the exception comes inside the commit_db_transaction method itself.

I think attempting to be well behaved in the presence of Thread#kill is going to be too much effort to be worthwhile.

@NZKoz NZKoz closed this May 9, 2011

@ghost ghost reopened this May 9, 2011

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 9, 2011

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz May 9, 2011

Member

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?

Member

NZKoz commented May 9, 2011

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?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 9, 2011

Did you read the post? It pretty much explains the issue exactly. tldr is all threads are Thread#killed when a process terminates.

ghost commented May 9, 2011

Did you read the post? It pretty much explains the issue exactly. tldr is all threads are Thread#killed when a process terminates.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz May 9, 2011

Member

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.

Member

NZKoz commented May 9, 2011

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 9, 2011

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).

ghost commented May 9, 2011

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).

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz May 9, 2011

Member

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

Member

NZKoz commented May 9, 2011

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

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 9, 2011

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.

ghost commented May 9, 2011

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.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Oct 9, 2011

Member

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?

Member

jeremy commented Oct 9, 2011

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?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 9, 2011

@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.

ghost commented Oct 9, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@mhfs

mhfs Feb 28, 2012

Contributor

@jeremy @Coderrr ping - this one seems to loose relevance with rails 4 being 1.9 only. wdyt? let's close it?

Contributor

mhfs commented Feb 28, 2012

@jeremy @Coderrr ping - this one seems to loose relevance with rails 4 being 1.9 only. wdyt? let's close it?

@jeremyf

This comment has been minimized.

Show comment
Hide comment
@jeremyf

jeremyf Apr 24, 2012

Contributor

@mhfs No responses for 2 months; close it.

Contributor

jeremyf commented Apr 24, 2012

@mhfs No responses for 2 months; close it.

@mhfs

This comment has been minimized.

Show comment
Hide comment
@mhfs

mhfs Apr 24, 2012

Contributor

@jeremyf I don't have permission to close it...

Contributor

mhfs commented Apr 24, 2012

@jeremyf I don't have permission to close it...

@jeremyf

This comment has been minimized.

Show comment
Hide comment
@jeremyf

jeremyf Apr 25, 2012

Contributor

@mhfs sorry about that…@Coderrr any follow-up?

Contributor

jeremyf commented Apr 25, 2012

@mhfs sorry about that…@Coderrr any follow-up?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 25, 2012

Member

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.

Member

rafaelfranca commented Apr 25, 2012

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.

@NZKoz NZKoz closed this Jun 19, 2012

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Jun 19, 2012

Member

Closing as we only support 1.9 now and the proposed changes here don't work there.

Member

NZKoz commented Jun 19, 2012

Closing as we only support 1.9 now and the proposed changes here don't work there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment