Thank you for this bug report.

On 13-Sep-2017, Darek Dzia=C5=82ak wrote:

Usually exclude_fds is empty or has few file descriptors. Right
now function iterate over all descirptors from
resource.getrlimit(resource.RLIMIT_NOFILE)[1] to 0

It's common to have exclude_fds non-empty, for example a log stream
for the daemon will need its file to remain open. The iteration to
close file descriptors must still skip the items in exclude_fds.

and attempt to close each file descriptor within python (most likely
receiving exception EBADF - Bad file descriptor).

You're right that daemon.close_all_open_files is less efficient than
os.closerange; it wasn't in the standard library when I wrote the
function, thank you for bringing it to my attention.

I think it could be solved in two ways:
1. use os.closerange

This still needs to exclude the items in exclude_fds, so it will
need to be customised or wrapped somehow to do that. Can you propose
an implementation for that?

1. close only opened file descriptors

Yes, that is exactly what daemon.close_file_descriptor_if_open is
intended to do. Because a file descriptor is merely an integer, there
does not appear to be a more efficient way to do this.

Do you know of a way that will still preserve the =E2=80=9Cclose the file
descriptor if it is open=E2=80=9D semantics?

--=20
\ =E2=80=9CFollowing fashion and the status quo is easy. Thinking ab=
out |
`\ your users' lives and creating something practical is much |
_o__) harder.=E2=80=9D =E2=80=94Ryan Singer,=
2008-07-09 |
Ben Finney ben@benfinney.id.au

I think it could be solved in two ways:
1. use os.closerange

This still needs to exclude the items in exclude_fds, so it will
need to be customised or wrapped somehow to do that. Can you propose
an implementation for that?

sure (attached also file close_fd_with_ranges.py with included some simple doctest)

import os
closerange = os.closerange
def close_all_open_files(exclude=None):
    min_fd = 3
    max_fd = get_maximum_file_descriptors()
    if not exclude:
        closerange(min_fd, max_fd)
        return
    for ex_fd in sorted(exclude):
        if ex_fd < min_fd:
            continue
        if ex_fd == min_fd:
            min_fd = ex_fd + 1
            continue
        closerange(min_fd, ex_fd)
        min_fd = ex_fd + 1
    if min_fd and min_fd < max_fd:
        closerange(min_fd, max_fd)

close only opened file descriptors

Yes, that is exactly what daemon.close_file_descriptor_if_open is
intended to do. Because a file descriptor is merely an integer, there
does not appear to be a more efficient way to do this.

not exactly... close_file_descriptor_if_open always closes file descriptor and catch an exception.
if fd is not open os.close raises an exception and it's time consuming. My proposition was to get list of opened file descriptors instead of iterating over all of them. For linux you can just check:
os.listdir('/proc/' + str(os.getpid()) + '/fd') but of course the fallback is need because some environments could have not mounted /proc or not accessible due to some admin restrictions.

On 10-Oct-2017, Darek Dzia=C5=82ak wrote:

This still needs to exclude the items in exclude_fds, so it will
need to be customised or wrapped somehow to do that. Can you
propose an implementation for that?
=20
sure (attached also file close_fd_with_ranges.py with included
some simple doctest)

I have re-written that, with unit tests. Please see merge request
https://pagure.io/python-daemon/pull-request/11 for my implementation.

You can also try branch =E2=80=98wip/issue/10/close-open-fds-with-closerang=
e=E2=80=99
https://pagure.io/python-daemon/branch/wip/issue/10/close-open-fds-with-clo=
serange
I would like to know whether it addresses this bug for you.

--=20
\ =E2=80=9CYou don't change the world by placidly finding your blis=
s =E2=80=94 |
`\ you do it by focusing your discontent in productive ways.=E2=80=
=9D |
_o__) =E2=80=94Paul Z. Myers, 2011-08=
-31 |
Ben Finney ben@benfinney.id.au

Thanks for solving that issue. I've added comments in PR.

On 17-Oct-2017, Darek Dzia=C5=82ak wrote:

Thanks for solving that issue. I've added comments in PR.

I don't know whether it solves the issue; it passes the tests I wrote,
but I don't have before-and-after comparison.

Can you get the code from that branch, and run it to verify that it
solves the reported issue for you?

--=20
\ =E2=80=9CFaith, n. Belief without evidence in what is told by one =
who |
`\ speaks without knowledge, of things without parallel.=E2=80=9D =E2=
=80=94Ambrose |
o__) Bierce, _The Devil's Dictionary, 1906 |
Ben Finney ben@benfinney.id.au

Howdy @ddzialak,

Can you get the code from that branch, and run it to verify that it
solves the reported issue for you?

Yes, It's much better then before (so it solves an issue). Just small comment is about unnecessary creation of python list with candidates (list from 0 up to hard limit for num of opened files). In my case max_fd is 1048576 and creating list with range function took around 30ms.

Anyway, because it's much better then before then I can say that yes, it solved an issue.
Thanks for working on that!

Anyway, because it's much better then before then I can say that yes, it solved an issue.
Thanks for working on that!

Thank you for the design of the fix. I've merged those changes now, and it will be in the next release.