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

Detect ZAI ZMM leaks #1229

Merged
merged 2 commits into from
May 13, 2021
Merged

Detect ZAI ZMM leaks #1229

merged 2 commits into from
May 13, 2021

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented May 11, 2021

Description

This PR adds a way to detect Zend Memory Manager leak messages in the ZAI tests by grepping the test log in CI for:

=== Total n memory leaks detected ===

There were a few ZMM leaks that were isolated to the tests (the leaks were not in the tracer or any production code paths) and those are fixed in d8b4c3e.

Ideally the leak detection would be implemented as part of the ZAI SAPI test runner, but every way I tired to implement it, I hit a blocker.

  • ZMM bypasses the SAPI error logger and writes the leak message directly to stderr which I haven't been able to capture from ZAI SAPI during RSHUTDOWN.
  • There is no way to inject a custom hook to capture the ZMM memory leak messages.
  • I cannot get Catch2 to capture stdout or stderr from ZAI SAPI using a custom event listener; ZmmLeakListener. The event listener works, but testCaseStats.stdOut and testCaseStats.stdErr are always empty from testCaseEnded.
  • Changing the Catch2 reporter to one that captures output, like junit, the custom ZmmLeakListener still cannot access ZAI SAPI stdout and stderr.
  • Changing ZAI SAPI write operations from write to fwrite does not seem to make any difference.
  • It seems as if the reporter doesn't even have access to the output streams; for junit, <system-out/> and <system-err/> are always empty.
  • There doesn't appear to be a way to run a post-test script using cmake.

That leaves me with "manually" grepping the test log for the ZMM leak message in CI. I'm probably missing something obvious with the Catch2 setup, so if anyone else can figure out a better solution to this, I'd love to know your thoughts.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

@SammyK SammyK added 🏆 enhancement A new feature or improvement dev/testing ci labels May 11, 2021
@SammyK SammyK added this to the 0.60.0 milestone May 11, 2021
@SammyK SammyK marked this pull request as ready for review May 11, 2021 22:54
Copy link
Collaborator

@morrisonlevi morrisonlevi 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 to me!

@@ -737,6 +737,8 @@ TEST_CASE("call static method: non-static method (userland)", "[zai_methods]") {
REQUIRE(retval != NULL);
REQUIRE(Z_TYPE_P(retval) == IS_BOOL);

zval_ptr_dtor(&retval);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this was found via the added memory leak detection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was. :)

@SammyK SammyK merged commit 6f1f4b2 into master May 13, 2021
@SammyK SammyK deleted the sammyk/zai-sapi/zmm-leak-detect branch May 13, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci dev/testing 🏆 enhancement A new feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants