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

Ensure shutdown function does not raise exception on ENOTCONN #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

francoisthire
Copy link

@francoisthire francoisthire commented Jan 3, 2025

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)

@anmonteiro
Copy link
Collaborator

we should be using Lwt.catch instead of try for Lwt functions.

@francoisthire
Copy link
Author

francoisthire commented Jan 4, 2025

I thought about using Lwt.catch, but the issue here is that it changes the type of the function. Indeed, the return type of shutdown is unit, and using Lwt.catch would turn it into unit Lwt.t. I am not familiar with this library, but it would look like a breaking change since the shutdown function is exposed. Let me know what is best.

@francoisthire
Copy link
Author

I have pushed a commit which changes the type of shutdown. Let me know if we should keep it.

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