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

tst: fix file descriptor leaks #5568

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Conversation

fingolfin
Copy link
Member

This is a step towards fixing the testinstall test suite on macOS, where the maximum number of file descriptors seems to exactly match our limit, so any leaked descriptor causes the 'too many open files' test in tst/testinstall/streams.tst to fail in an unexpected way (namely because instead of the expected error, OutputTextFile returns 'fail').

Note that just executing the tests in tst/testinstall/streams.tst was and is fine, but running the full testinstall test suite still is not; I suspect there are some remaining fd leaks in IO and/or curlInterface, triggered by atlasrep using Download from the utils package... sigh.

@fingolfin fingolfin added topic: tests issues or PRs related to tests release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 9, 2024
This is a step towards fixing the `testinstall` test suite on macOS,
where the maximum number of file descriptors seems to exactly match our
limit, so any leaked descriptor causes the 'too many open files' test in
`tst/testinstall/streams.tst` to fail in an unexpected way (namely
because instead of the expected error, OutputTextFile returns 'fail').
@fingolfin
Copy link
Member Author

@ChrisJefferson since I am sure you are curious, I am using DirectoryContents("/dev/fd"); as a hack to see which file descriptors are open. In a fresh GAP session, initially we can see 5 fds: stdin, stdout, stderr, "lib/init.g", and the directory "/dev/fd"

gap> DirectoryContents("/dev/fd");
[ "0", "1", "2", "3", "4" ]

By accessing the atlasrep variable AtlasOfGroupRepresentationsInfo, which is marked as "auto loading", I can get a couple more "wasted" descriptors:

gap> AtlasOfGroupRepresentationsInfo;;
gap> DirectoryContents("/dev/fd");
[ "0", "1", "2", "3", "4", "7", "10", "11" ]

This is with curlInterface loaded and used as download method. If curlInterface is not available, then I only get

gap> DirectoryContents("/dev/fd");
[ "0", "1", "2", "3", "4", "7" ]

The extra descriptors all appear to be sockets, but I have not yet figured out why they are left around. Perhaps you have an idea.

@ChrisJefferson
Copy link
Contributor

I'll have a look, and see what I find.

@ChrisJefferson
Copy link
Contributor

So only linux I get 6 as base, I also get '6', which is /dev/ptmx (something to do with terminals, not sure which package is making it). However, I even with curlInterface loaded I don't get any new descriptors from AtlasOfGroupRepresentationsInfo, even with curlInterface loaded. I also tried running the curlInterface test suite, and nothing came up.

I had a look in IO, and there is only leaks in one test which purposefully leaks some open files (we could comment out that test and see if it's causing a problem).

@ChrisJefferson
Copy link
Contributor

After the fixes in this branch, I don't see any more leaks -- every test in teststandard and testinstall seems to end with as many open files as it started with (assuming my quickly hacked up script is correct)

@fingolfin
Copy link
Member Author

ptmx = pseudo terminal multiplexer, this is likely opened as a result of us launching and controlling subprocesses via openpty / posix_openpt.

Unfortunately on macOS I still see the leaks and testinstall still is consistently failing for me :-(. But of course that shouldn't stop us from merging this PR as it still is an improvement.

@fingolfin fingolfin merged commit e00b01f into gap-system:master Jan 10, 2024
22 checks passed
@fingolfin fingolfin deleted the mh/fd-leaks branch January 10, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants