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

Tests that no Windows handles are leaked in Directory APIs #99

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Aug 15, 2020

Hi @mosra !

As per gitter, here's the fix for the leaking handle. Windows will refuse to delete folders with open handles, which means every list()-ed folder would be locked until application exit.

Best,
Jonathan


Edit by @mosra -- things to do in addition to Windows handle leak counting because yes everything turns into a "fixing a lightbulb" problem after a while:

  • Make it possible to have checks in a teardown routine (I have branch for this somewhere)
  • Turn this into a teardown check that's attached for test cases that access the OS APIs
  • Figure out a way to check file descriptor leaks on Unix as well (mainly for tests that map empty files, etc.)
  • Checking that we're errno clean in Directory APIs / tests (in particular, if some API such as rm() fails, it should clean errno after itself)

Signed-off-by: Squareys <squareys@googlemail.com>
#ifdef CORRADE_TARGET_WINDOWS
/* Ensure we are not leaking handles */
DWORD newHandleCount = 0;
GetProcessHandleCount(GetCurrentProcess(), &newHandleCount);
Copy link
Owner

Choose a reason for hiding this comment

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

Wow, this is cool, didn't even know such thing was possible.

You know what? This would be good to do for everything -- I think this could be doable in setup/teardown routines, but I need to patch TestSuite a bit to allow checks there. Is it okay if I merge just the first part with the scope guard so you have the fix and leave this extra check for later when TestSuite is ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also merge this and refactor it once test suite is ready (or refactor with making it ready).

@mosra mosra added this to TODO in Utility via automation Aug 15, 2020
@mosra mosra added this to the 2020.0b milestone Aug 15, 2020
@mosra
Copy link
Owner

mosra commented Aug 15, 2020

Merged the fix alone in 515cc7e, keeping this open so I have a reminder to add the handle counting to all tests.

Thanks!

@mosra mosra changed the title Directory: Close a leaking handle in list() and test Tests that no Windows handles are leaked in Directory APIs Aug 15, 2020
@mosra mosra marked this pull request as draft August 15, 2020 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Utility
  
TODO
Development

Successfully merging this pull request may close these issues.

None yet

2 participants