Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Feb 1, 2019

These happen randomly on CI sometimes.

I believe what happens is that each test looks if anything changed in /tmp/. For that reason we already ignore /tmp/tmpaddon, which is added by either the CI or one of the browsers - it gets added while a test runs, and the test thinks it's a leak. The problem this PR solves is that the canonical temp dir will stay alive after a test finishes, and so if one test begins before it exists and finishes after it, it will look like a leak. (I think we don't see this more often because the dir is created fairly early on.) In any case, the canonical temp and test dirs are not things that can "leak", they are meant to stay alive, so just ignore them for leak test purposes.

@kripken kripken requested a review from sbc100 February 1, 2019 03:52
@sbc100
Copy link
Collaborator

sbc100 commented Feb 1, 2019

Hmm. . I need to look into this a little more but my first reaction is that should not be needed. The parallel test runner should set a unique tmp dir for each test that runs, so these directories should not get used during parallel testing.

The canonical "emscripten_temp" directory in this case would end up somewhere like /tmp/tmp_dir_single_teat/emscripten_temp. And only certain tests will ever write to this and they are marked with @uses_canonical_temp

The canonical "emscripten_test" directory is only ever used when EMTEST_SAVE_DIR=1 is set and this is never set on the bots.

@kripken
Copy link
Member Author

kripken commented Feb 1, 2019

You're right, I wasn't looking closely enough at the error message - it does indeed say that /tmp/tmp_dir_single_teat/emscripten_temp is leaked. I'm not sure why that happens, then...

@kripken
Copy link
Member Author

kripken commented Feb 1, 2019

This seems to happen all the time now on the bots, on upstream other.test_debuginfo: https://circleci.com/gh/emscripten-core/emscripten/81190?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Odd it's only on upstream. Doesn't reproduce locally.

@kripken
Copy link
Member Author

kripken commented Feb 4, 2019

@sbc100 ok, I figured this out, and replaced the code in this PR with a proper solution. What happened is that

  • In tests using the canonical temp dir, if the test failed, we didn't get to the point where we clean up the canonical temp dir. And if leak detection code was on, that would fire. So when such a test failed in such a situation, it would not report the test failure, and only a leak.
  • I mistook that leak for a random failure, but it was a real issue caused by the logging change I made - it did not log out properly in python 3.
  • This PR fixes that logging, removes some redundant logging after that, and fixes tests that depend on our debug logging to pass again.
  • Also fixes cleaning up the canonical temp dir in tests that use it, to fix that problem of a leak being reported instead of the error.

@juj
Copy link
Collaborator

juj commented Feb 4, 2019

Looks good to me, I'll push this in so that CI can show green on PRs again.

@juj juj merged commit c45433c into incoming Feb 4, 2019
@kripken kripken deleted the leaks branch February 4, 2019 18:45
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.

4 participants