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

Lwt-unix: Fix file descriptor leak with Lwt_unix.shutdown #81

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

Conversation

francoisthire
Copy link

I stumbled across a case where the syscall shutdown of my application failed with ENOTCONN and the syscall to close did not occur resulting into a file descriptor leak.

By looking at the potential culprit, I found the shutdown function of this library.

The patch is inspired to what is done in lwt_io:

let close_socket fd =
  Lwt.finalize
    (fun () ->
       Lwt.catch
         (fun () ->
            Lwt_unix.shutdown fd Unix.SHUTDOWN_ALL;
            Lwt.return_unit)
         (function
           (* Occurs if the peer closes the connection first. *)
           | Unix.Unix_error (Unix.ENOTCONN, _, _) -> Lwt.return_unit
           | exn -> Lwt.reraise exn))
    (fun () ->
       Lwt_unix.close fd)

(for reference, another similar MR: ocsigen/lwt_ssl#5)

@anmonteiro
Copy link
Owner

it makes sense, but can you reproduce the fix in lwt_io faithfully? we shouldn't be using try with Lwt

@francoisthire francoisthire force-pushed the francois@fix-fd-leak-on-shutdown branch from 86c0782 to 42ad767 Compare January 4, 2025 10:11
@francoisthire
Copy link
Author

I have force-pushed the commit mimicking the lwt_io function. I believe that for this particular case it does not change anything since the shutdown function is not in lwt though. So the try ... with will work as expected.

@anmonteiro
Copy link
Owner

I believe that for this particular case it does not change anything since the shutdown function is not in lwt though. So the try ... with will work as expected.

I agree but it required going looking for the implementation to know. That shouldn’t be required.

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.

3 participants