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

[Row] Fix: db connection leakage in Err method #846

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chhekur
Copy link

@Chhekur Chhekur commented Jan 22, 2023

Issue Outline: there is db connection leakage happening here, if someone makes a DB calls and doesn't really call any of the Scan methods where rows generally being closed via calling rows.Close() method and directly calls the Err() method over Row instance just to check the err in that case rows cursor connection is still alive and hence it leaks the db connection.

Proposed Solution: explicitly close the rows in the Err() method too so that it can be made sure that if someone directly calls this method without using any scanning method, still connection would be closed.

@dlsniper
Copy link
Collaborator

dlsniper commented Feb 1, 2024

Hello, @ardan-bkennedy, and I recently stepped in to help maintain this project.
We are sorting the opened issues and pull requests and would like to know if you still NEED this merged.
Thank you for your contribution.

I'm tempted to say that this is not a proper solution to the problem, as calling the method would always close the connection. This will break code that does a check using the method.

I think the solution for this would be to add a mention in the method documentation and mention that Close needs to be called when the error is not nil. What do you think?

@dlsniper dlsniper added the requires attention The PR looks like it could potentially break things. Definitely requires more testing label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires attention The PR looks like it could potentially break things. Definitely requires more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants