Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding changes in #335 by @kayslay to handle transaction panic #650

Closed
wants to merge 1 commit into from

Conversation

paganotoni
Copy link
Member

@paganotoni paganotoni commented May 12, 2021

Original comment was:

Handle panic that comes from the fn func(tx *Connection) parameter.

Whenever a panic occurs in a Connection.Transaction the TX.Rollback or TX.Commit is not called after the panic has been recovered. Any call to the Connection.Transaction would block forever.

Thanks @kayslay

cc @stanislas-m @kayslay

@paganotoni paganotoni requested a review from a team as a code owner May 12, 2021 18:52
@paganotoni paganotoni changed the title Adding changes in #335 by @kaylay to handle transaction panic Adding changes in #335 by @kayslay to handle transaction panic May 12, 2021
@aeneasr
Copy link
Member

aeneasr commented May 13, 2021

Shouldn't we rather look into what's causing the panic?

@paganotoni
Copy link
Member Author

Ah! Fair point. Honestly I'm not sure how the underlying layers of transaction work in Pop and just wanted to bring this PR up in case it still relevant. Will take a look.

@aeneasr
Copy link
Member

aeneasr commented May 13, 2021

It's also possible that the user is running into panics in her/his code and the panic just bubbles up. In that case however I don't think that pop should be the one recovering it but rather the user's middleware for example. Did the original issue include a stack trace for the panic?

@paganotoni
Copy link
Member Author

It did not, lets just close this one and maybe take a look when we have a concrete case. So far I don't have a way to reproduce it. Thanks @aeneasr

@paganotoni paganotoni closed this May 14, 2021
@sio4
Copy link
Member

sio4 commented Jan 13, 2023

I found this closed PR during branch cleanup. The same issue was addressed with two PRs below, and there are also related fixes on Buffalo core side too. (leaving this comment just for a note with references)

@sio4 sio4 deleted the task-transaction-panic branch January 13, 2023 15:37
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.

None yet

3 participants