ocurrent / solver-service

An OCluster service for solving opam dependencies
Apache License 2.0
12 stars 7 forks source link

File descriptor leaks #39

Closed maiste closed 1 year ago

maiste commented 1 year ago

This PR intends to fix the Unix.EMFILE that happens when too many file descriptors are opened simultaneously.

It tackles the problem at various places in the code, as suggested by @TheLortex (Thanks :pray: ):

:warning: This is a PR that has been tested with a local setup so it needs to be deployed at some point to make sure it fixes the problem.

moyodiallo commented 1 year ago

Thanks, this is good :+1: .

Some to complete: Those lines are where the internal workers are created. Each time a internal worker is spawn we have 2 pipes for child process and 2 other pipes for the parent process.

https://github.com/maiste/solver-service/blob/f125f8bc97e20e027920fc1052ee69894c9b38bc/service/main.ml#L106 https://github.com/maiste/solver-service/blob/f125f8bc97e20e027920fc1052ee69894c9b38bc/service/main.ml#L73

maiste commented 1 year ago

Thanks, this is good +1 .

Some to complete: Those lines are where the internal workers are created. Each time a internal worker is spawn we have 2 pipes for child process and 2 other pipes for the parent process.

https://github.com/maiste/solver-service/blob/f125f8bc97e20e027920fc1052ee69894c9b38bc/service/main.ml#L106 https://github.com/maiste/solver-service/blob/f125f8bc97e20e027920fc1052ee69894c9b38bc/service/main.ml#L73

The internal worker should close its connections when it calls dispose here.

moyodiallo commented 1 year ago

Thanks @maiste and @TheLortex. The PR is tested with more pressure using https://github.com/ocurrent/solver-service/pull/41 and no crash noticed.

~We can revert this PR now https://github.com/ocurrent/ocaml-ci/pull/742 @tmcgilchrist.~