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

pop.Connection.Transaction can leak connections #748

Closed
Tracked by #770
chrisseto opened this issue Jul 19, 2022 · 1 comment · Fixed by #775
Closed
Tracked by #770

pop.Connection.Transaction can leak connections #748

chrisseto opened this issue Jul 19, 2022 · 1 comment · Fixed by #775
Assignees
Labels
bug Something isn't working s: fixed was fixed or solution offered
Milestone

Comments

@chrisseto
Copy link

Description

If a user's code panics within a pop.Connection.Transaction closure, .Rollback nor .Commit will not be called resulting in a leaked SQL connection.

Steps to Reproduce the Problem

func DoThing(conn *pop.Connection) {
    _ = conn.Transaction(func(tx *pop.Connection) error {
        panic("Oh no, this is probably due to a null pointer exception!")
    })
}

Expected Behavior

.Transaction should recover panics and attempt to call .Rollback before re-panicking the error.

Actual Behavior

A SQL connection is silently leaked. Depending on your connection pool setting, you might not even notice.

Info

This issue is not specific to an OS and is observable on the latest main commit (4e72fd8) as of writing

@sio4
Copy link
Member

sio4 commented Jul 20, 2022

Thank you for reporting the issue. It could be a serious issue if the transaction is not guaranteed even though the root cause is on the user's own code. Will check the sequence but it could take more time.

@sio4 sio4 added security bug Something isn't working s: triage Some tests need to be run to confirm the issue labels Sep 20, 2022
@sio4 sio4 added this to the v6.0.7 milestone Sep 20, 2022
@sio4 sio4 mentioned this issue Sep 20, 2022
30 tasks
@sio4 sio4 self-assigned this Sep 22, 2022
@sio4 sio4 removed the s: triage Some tests need to be run to confirm the issue label Sep 22, 2022
@sio4 sio4 closed this as completed in #775 Sep 24, 2022
@sio4 sio4 added the s: fixed was fixed or solution offered label Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working s: fixed was fixed or solution offered
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants