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.

Sign up for GitHub

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

Jump to bottom

Connection.runInteraction() may not close cursor? #35

Closed
mrwonko opened this issue May 1, 2015 · 3 comments
Closed

Connection.runInteraction() may not close cursor? #35

mrwonko opened this issue May 1, 2015 · 3 comments

Comments

@mrwonko
Copy link
Contributor

mrwonko commented May 1, 2015

I just read through the source for runInteraction() since the documentation left me wondering whether I was supposed to manually close the cursor in the supplied function. (Apparently not.)

If I'm reading this right, rollbackAndPassthrough() will always return a Failure, causing the callback that closes the cursor to never be called since the errback will be called instead? Shouldn't it be addBoth() instead of addCallback() for the c.close() lambda? Or am I missing something?
@wulczer
Copy link
Owner

wulczer commented May 4, 2015

Ah, you're right, it's not closing the cursor in case of an error. I don't think the last addCallback() should be an addBoth() but there should be a close() call inside rollbackAndPassthrough().

Good catch, will fix.

@wulczer wulczer closed this as completed in d236d3b May 4, 2015
@wulczer
Copy link
Owner

wulczer commented May 4, 2015

Thanks for the report, I made the docs clearer on that and made sure the cursor is always closed.

Oh, and turned out you were right, it should have been addBoth() :)

@mrwonko mrwonko mentioned this issue May 4, 2015
Closed
@mrwonko
Copy link
Contributor Author

mrwonko commented May 4, 2015

Beware that this probably broke disconnecting during interaction execution, see #37 for how fixing runQuery() this way broke the disconnectWhileRunning test.

wulczer added a commit that referenced this issue May 4, 2015
@wulczer
Do not close internally used cursors at all.
7ecefe5 
The only reason for closing a psycopg2 cursor is to prevent its further
usage. Cursors created internally when using runQuery and runOperation are
never surfaced to the user, so there's no need to explicitly close them.

Issue #35 spurred an effort to make sure all generated cursors are always
closed, but it now seems that it's not the right way to go about the
problem. There's no benefit to forcing psycopg2 cursors to be closed and if the
cursor (or connection) are in a bad state, it risks throwing suprious errors.

Leave it to psycopg2 to deal with cursors going away and simply fix
runInteraction's docstring to explicitly say that the user should not close the
cursor they got passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mrwonko @wulczer