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

CA-364138 XSI-1217: fix FD leak, Unix.EMFILE #4675

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

lindig
Copy link
Contributor

@lindig lindig commented Apr 8, 2022

  • Fix file descriptor leak
  • Log the number of file descriptors in use when serving a new VM - this is optional but helps to verify the fix
  • Currently testing this (after having removed the debugging code that I used to find the leak)

Close both FDs.

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
@lindig lindig force-pushed the private/christianlin/CA-364138 branch from 91b05e1 to 05e22a4 Compare April 8, 2022 15:38
@lindig lindig marked this pull request as ready for review April 8, 2022 15:38
@lindig
Copy link
Contributor Author

lindig commented Apr 8, 2022

Processing  1/1: [default: git]
Error:  Could not update repository "default": "/usr/bin/git fetch -q" exited with code 128 "fatal: unable to access 'https://github.com/xapi-project/xs-opam.git/': The requested URL returned error: 429"
Error:  Initial download of repository failed
'/opt/hostedtoolcache/opam/2.0.8/x64/opam init --bare -yav https://github.com/xapi-project/xs-opam.git' failed.
Error: Error: The process '/opt/hostedtoolcache/opam/2.0.8/x64/opam' failed with exit code 40

HTTP error 429 would indicate a rate limit. So: not a code problem so far.

@@ -22,6 +22,11 @@ let ret v = v >>= Lwt.return_ok |> Rpc_lwt.T.put

let sockets = Hashtbl.create 127

let log_fds () =
let fds = Sys.readdir "/proc/self/fd" |> Array.length in
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use Lwt_unix.files_of_directory and then count the number of entries in the Lwt stream if we want to avoid blocking here (Sys.readdir would be a blocking call that blocks all Lwt promises)

Copy link
Contributor

@edwintorok edwintorok Apr 8, 2022

Choose a reason for hiding this comment

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

let fds = Lwt_unix.files_of_directory "/proc/self/fd" |> Lwt_stream.to_list >|= List.length in

or

let count_stream s = Lwt_stream.fold (fun _ acc -> acc + 1) s 0 in
let fds = Lwt_unix.files_of_directory "/proc/self/fd" |> Lwt_stream.to_list >|= List.length in

Copy link
Contributor

Choose a reason for hiding this comment

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

(there are a few more places in varstored_guard that use blocking calls like Sys.file_exists, don't worry about those I'll try to fix those in my PR)

Copy link
Member

Choose a reason for hiding this comment

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

I think you meant

let stream_length s = Lwt_stream.fold (fun _ acc -> acc + 1) s 0 in
let fds = Lwt_unix.files_of_directory "/proc/self/fd" |> stream_length

@lindig
Copy link
Contributor Author

lindig commented Apr 11, 2022

The point about blocking is good. On the other hand: the /proc file system is implemented in the kernel and this is called only once we serve a new VM.

After starting a new server (for a varstored), report the number of open
file descriptors in the log.

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
@lindig lindig force-pushed the private/christianlin/CA-364138 branch from 44e79de to a4b7d64 Compare April 11, 2022 08:43
@lindig lindig merged commit 2f5abb7 into xapi-project:master Apr 11, 2022
@lindig
Copy link
Contributor Author

lindig commented Apr 11, 2022

Small addendum: the change to use Lwt_unix.files_of_directory leads to a slight inaccuracy because it reports . and .. as well, which the previous code did not report.

+let log_fds () =
+  let count stream = Lwt_stream.fold (fun _ n -> n + 1) stream 0 in
+  Lwt_unix.files_of_directory "/proc/self/fd" |> count >>= fun fds ->
+  D.info "file descriptors in use: %d" fds ;
+  Lwt.return_unit

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