Skip to content

Fix incomplete transaction commit when thread is shutdown ungracefully#704

Merged
larskanis merged 3 commits into
ged:masterfrom
intale:transaction_issue
May 16, 2026
Merged

Fix incomplete transaction commit when thread is shutdown ungracefully#704
larskanis merged 3 commits into
ged:masterfrom
intale:transaction_issue

Conversation

@intale
Copy link
Copy Markdown
Contributor

@intale intale commented May 16, 2026

Problem description

Running transactions may suddenly commit when the thread it is running from is force-shutdown.

Steps to reproduce

Refer to the newly added test case

Solution

To fix this we have to additionally check the thread status in ensure block and rollback in case we've been interrupted.

@intale
Copy link
Copy Markdown
Contributor Author

intale commented May 16, 2026

It seems I need to look into the behaviour of truffleruby as, it seems, it requires special attention

Comment thread spec/pg/connection_spec.rb Outdated
Comment thread spec/pg/connection_spec.rb Outdated
Comment thread spec/pg/connection_spec.rb Outdated
Comment thread lib/pg/connection.rb
@intale intale force-pushed the transaction_issue branch from a3eb075 to c3d2aab Compare May 16, 2026 11:57
@intale intale requested a review from larskanis May 16, 2026 12:18
larskanis added 2 commits May 16, 2026 17:41
It seems to be a Truffleruby bug, that blocking IO reverts Thread#status back to "run".
So avoid blocking IO, but use Kernel.sleep instead.
See here for a similar issue on JRuby: jruby/jruby#4705
@larskanis larskanis force-pushed the transaction_issue branch from d425783 to 2a11006 Compare May 16, 2026 15:42
Copy link
Copy Markdown
Collaborator

@larskanis larskanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two more commits. Are they OK from your point of view?

@intale
Copy link
Copy Markdown
Contributor Author

intale commented May 16, 2026

I added two more commits. Are they OK from your point of view?

I have a concern regarding rescue Exception => error case. Previously it was rescue => rollback => re-raise which was logical, but now it is rescue => re-raise => rollback. While I can't provide any edge case where this sequence change may result in any side effect - it may depend on a different ruby implementation, thus introducing some different behaviour than intended. What do you think? Do you have something in mind that could potentially affect on the final result? Otherwise looks good to me - you caught this randomly failing issue with the test I provided and replaced pg_sleep with sleep.
Imho, I would keep the implementation explicit for each case like it was before as it is more straightforward from my point of view.

@larskanis
Copy link
Copy Markdown
Collaborator

I like the new ensure based approach more, since rollback in rescue makes it more cluttered. This is because rescue is not called in two cases: when the yield is aborted by break and when the thread is killed. But all cases are caught in the ensure branch. So it's more natural and clearer to me to handle all cases of commit or rollback there. The ruby language definition of rescue/ensure is old and well described - it's not an edge case.

@intale
Copy link
Copy Markdown
Contributor Author

intale commented May 16, 2026

The ruby language definition of rescue/ensure is old and well described - it's not an edge case.

It is. Cool. I just tried to be paranoid about the potential behaviour. I don't have any more remarks.

@larskanis larskanis merged commit 59296b0 into ged:master May 16, 2026
60 of 61 checks passed
@larskanis
Copy link
Copy Markdown
Collaborator

Thank you @intale for raising this issue and providing fix and test!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants