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

Fix segmentation faults with enabled keep_descriptor_pool_after_request #16993

Conversation

simonberger
Copy link
Contributor

Fixes #16894

It is tested on command execution only, where it fixes the issues of reported memory leaks, heap corruption and segmentation fault.

I could not run tests on my Mac because of #16944

@simonberger simonberger requested a review from a team as a code owner May 31, 2024 11:13
@simonberger simonberger requested review from bshaffer and removed request for a team May 31, 2024 11:13
@hakimio
Copy link

hakimio commented Jun 13, 2024

@haberman take a look?

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 13, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 13, 2024
@haberman
Copy link
Member

Hmm, looks like there are some test failures unrelated to your change. Want to rebase and push again?

@simonberger simonberger force-pushed the bugfix/php-ext-persistent-global-corruption branch from 75847ac to 62b5529 Compare June 15, 2024 00:23
@simonberger
Copy link
Contributor Author

Hmm, looks like there are some test failures unrelated to your change. Want to rebase and push again?

@haberman Sure, done

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 16, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 16, 2024
@MrMage
Copy link

MrMage commented Jun 17, 2024

Looks like only some internal "copybara" check failed. Is that a blocker, or can this be merged?

@bshaffer would it be possible for you to merge this in your function as an owner?

@haberman
Copy link
Member

All PRs must be committed internally first, because the internal repo is the source of truth. If we tried to merge the PR directly in GitHub I think it might get reverted by the next CL that syncs to GitHub.

The Copybara failure should be temporary. Internally the CL is in @honglooker's court to review and resolve any issues with the internal CI.

@MrMage
Copy link

MrMage commented Jun 18, 2024

Thank you for the insights! Looks like this was now merged internally and synced by Copybara.

@simonberger
Copy link
Contributor Author

simonberger commented Jun 25, 2024

I already asked in the related issue about if a backport to 25.x should be done what I personally do not really care about, but I just realized this fix currently would not do it in a potential 27.2 release.
Is this something you do internally or shall I create a backport fix for 27.x and possibly 25.x. Some more clear expectations for the different types of pull requests would be good. I read in the guide everything shall target main first which is unlike most other repositories I know. What shall follow after is a bit uncertain to me.

Edit:
Created backport to 27.x here #17232 in case it is not required/wanted just close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PHP] Heap Corruption when keep_descriptor_pool_after_request is active
4 participants