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

[Tests] Avoid memory leak bug with msodbcsql #12615

Merged
merged 1 commit into from
Nov 7, 2023
Merged

[Tests] Avoid memory leak bug with msodbcsql #12615

merged 1 commit into from
Nov 7, 2023

Conversation

SakiTakamachi
Copy link
Member

Please see #12306 (comment)

@SakiTakamachi SakiTakamachi mentioned this pull request Nov 5, 2023
Merged
@SakiTakamachi
Copy link
Member Author

Deadlock? why?

mvorisek
mvorisek reviewed Nov 5, 2023
View reviewed changes
ext/odbc/tests/odbc_connect_001.phpt Outdated
@@ -16,35 +16,40 @@ include 'skipif.inc';
include 'config.inc';
$dsn = str_replace(";uid={$user};pwd={$pass}", '', $dsn);

/*
* A bug in msodbcsql causes a memory leak when reconnecting after closing.
Copy link
Contributor

Choose a reason for hiding this comment

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

xfail with link to a bug tracker might be better

Copy link
Member

Choose a reason for hiding this comment

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

I also wouldn't comment all the close statements etc. We have xleak to handle this specifically. I'd advise to keep the test running when not under ASAN and use xleak + a link to the bugtracker.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nielsdos @mvorisek
Thank you.

This memory leak is not detected in normal debug mode and requires the following option:

php-src/.github/workflows/nightly.yml

Line 352 in 4022e28

CFLAGS='-fsanitize=undefined,address -fno-sanitize-recover -DZEND_TRACK_ARENA_ALLOC'

Will XLEAK still work in this case? In my Local environment, the test was treated as Failed even if I specified XLEAK.

@SakiTakamachi
Copy link
Member Author

A completely unrelated test is failing. Time-wise, it seems like it's a time zone issue, but I haven't looked into it in detail yet.

nielsdos
nielsdos approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Looks good. Have you reported this to upstream yet? If not, might be worth doing so.
I'm just wondering a bit whether we should do some msodbc-specific cleanup when reconnecting, as I'm surprised by this leak bug...

@SakiTakamachi
Copy link
Member Author

Thank you! I'm trying to find place to report the issue to, so I haven't report it yet. I will report as soon as I find it.

This bug surprises me too. It may be a good idea to do a workaround, because it may not be fixed right away even if I report.

@SakiTakamachi
Copy link
Member Author

SakiTakamachi commented Nov 7, 2023 •
edited
Loading

I don't know if this is the right place to report it, I posted it on the forum.
https://learn.microsoft.com/en-us/answers/questions/1418766/memory-leak-in-microsoft-odbc-driver-17-10-5-for-s

I also posted it here.
microsoft/msphpsql#1488

@nielsdos nielsdos merged commit 61c251d into php:master Nov 7, 2023
8 checks passed
@nielsdos
Copy link
Member

nielsdos commented Nov 7, 2023

Alright, thanks. Hopefully we'll hear something soon.

@SakiTakamachi SakiTakamachi deleted the tmp-fix/leak-on-odbc-test branch November 21, 2023 07:18
@SakiTakamachi
Copy link
Member Author

@iluuu1994
From MS's answer, I learned that the leak occurs with ODBC Driver for SQL Server 17, but not with 18. (17 is planned to be fixed, but it will not be released until next year.)

(Just changing it to 18 is not enough, you need to add a little bit to the DSN.)

I'm trying to change the driver to 18 in GitHub Actions, but I don't know where the 17 driver is being prepared in the first place. If you know anything, please let me know.

@SakiTakamachi
Copy link
Member Author

Apparently, it is included in the ubuntu image in github actions from the beginning.

And since msodbc18 is not yet compatible with ubuntu22.04, it may not be possible to remove XLEAK for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvorisek mvorisek mvorisek left review comments

@nielsdos nielsdos nielsdos approved these changes

Assignees
No one assigned
Labels
Extension: odbc Extension: pdo_odbc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SakiTakamachi @nielsdos @mvorisek