Skip to content

Don't leave cursors open - #23993

Merged
MorrisJobke merged 1 commit into
masterfrom
bugfix/noid/close-cursors
Nov 10, 2020
Merged

Don't leave cursors open#23993
MorrisJobke merged 1 commit into
masterfrom
bugfix/noid/close-cursors

Conversation

@nickvergessen

@nickvergessennickvergessen commented Nov 9, 2020

Copy link
Copy Markdown
Member

Signed-off-by: Joas Schilling coding@schilljs.com

@nickvergessennickvergessen added bug 3. to review Waiting for reviews labels Nov 9, 2020
@nickvergessennickvergessen added this to the Nextcloud 21 milestone Nov 9, 2020
@nickvergessennickvergessen changed the title Don't leave cursors open when tests failDon't leave cursors openNov 9, 2020
@nickvergessen

nickvergessen commented Nov 9, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable20

rullzer
rullzer approved these changes Nov 9, 2020
View reviewed changes

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks sane.
🚀

kesselb
kesselb approved these changes Nov 9, 2020
View reviewed changes

@kesselbkesselb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice 👍

Comment threadlib/private/legacy/OC_DB_StatementWrapper.php
Comment threadlib/private/legacy/OC_DB_StatementWrapper.php Outdated
@nickvergessen
Don't leave cursors open when tests fail
8027dcb
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
nickvergessenforce-pushed the bugfix/noid/close-cursors branch from c59ae47 to 8027dcbCompareNovember 9, 2020 11:29
@icewind1991

icewind1991 commented Nov 9, 2020

Copy link
Copy Markdown
Member

closing the cursor before a return makes no functional difference and only clutters the code, the cursor will be closed automatically when the result goes out of scope.

it only makes sense to manually close cursors if there is significant work being done in a function after the results are fetched

@MorrisJobke

MorrisJobke commented Nov 9, 2020

Copy link
Copy Markdown
Member

closing the cursor before a return makes no functional difference and only clutters the code, the cursor will be closed automatically when the result goes out of scope.

Was about to ask this as well. It looks really odd that we need to actively close the cursor there. Does this really fix the OCI cursor leak?

@nickvergessen

nickvergessen commented Nov 10, 2020

Copy link
Copy Markdown
MemberAuthor

Well at least the OC_DB methods leave cursors behind I don't know why and how, maybe because of the static-ness.

@nickvergessen

nickvergessen commented Nov 10, 2020

Copy link
Copy Markdown
MemberAuthor

And then I think it doesn't hurt and we should just make it a pattern in our code base. So when someone adds a second query or some code after your query they dont need to magically remember to add a closeCursor for the query above

MorrisJobke
MorrisJobke approved these changes Nov 10, 2020
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fine by me

@MorrisJobke
MorrisJobke merged commit f23c216 into masterNov 10, 2020
@MorrisJobke
MorrisJobke deleted the bugfix/noid/close-cursors branch November 10, 2020 14:15
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Nov 10, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzerrullzerrullzer approved these changes
@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@ChristophWurstChristophWurstChristophWurst approved these changes
@kesselbkesselbkesselb approved these changes
@icewind1991icewind1991Awaiting requested review from icewind1991
@juliusknorrjuliusknorrAwaiting requested review from juliusknorr

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbug

Projects

None yet

Milestone

Nextcloud 21

Development

Successfully merging this pull request may close these issues.

6 participants

@nickvergessen@icewind1991@MorrisJobke@rullzer@ChristophWurst@kesselb