janestreet/shexp

eval does not close context properly ?

Closed this issue · 3 comments

I'm not sure if this is a bug, but this code :

let loop n =    
  let open Shexp_process in                                                                        
    let rec loop k =                                    
      if k = 0 then ()                                                                              
      else                                                                
        (Printf.printf "iter %d" (n - k); print_newline ();                                           
         let context = Context.(create ~cwd:(Path "/tmp") ()) in    
         let () = eval ~context @@ run "echo" ["foo"] in    
         loop (k - 1)) in    
    loop n         

let () = loop 1024

Triggers an unix EMFILE exception as it creates too many files and never closes them (on my system , ulimit -Sn => 1024) . In contrast, this code runs fine :

let loop n =    
  let open Shexp_process in                                                                        
  let context = Context.(create ~cwd:(Path "/tmp") ()) in    
    let rec loop k =                                    
      if k = 0 then ()                                                                              
      else                                                                
        (Printf.printf "iter %d" (n - k); print_newline ();                                           
         let () = eval ~context @@ run "echo" ["foo"] in    
         loop (k - 1)) in    
    loop n         

let () = loop 1024

Because Context.create is only called once. Intuitively, I guess the correct behaviour would be that eval would close whatever file is opened by Context.create ? I'm absolutely not sure about that, maybe we want context to stay valid across runs, but it seems that it should be closed at some point, there is some resource leak going on here. Either way, I'm interested by your thoughts on that, thanks

Context.create always open a new file descriptor for the cwd, and keeps it open in case the context is used again. This should probably be documented. I guess this is what is causing the leak.

Context.create always open a new file descriptor for the cwd, and keeps it open in case the context is used again. This should probably be documented. I guess this is what is causing the leak.

Oh sorry, I was not aware that a Context.dispose function exists, that is also a way to solve my problem. Is there any reason you're not using Gc.finalise to call it automatically (it solves my problem) ? Maybe because it's not reliable enough ?

Indeed, Gc.finalise is not a great way to manage open file descriptors. The amount of open fds might exceed the limit before the next gc for instance. As such, it is more robust to dispose of the context manually.

I'm closing this issue then.