Poolex - A library for managing pools of workers

Your Libraries & OS Mentoring Libraries
, ,
12

Hi, nice library, I’ve read the code and I have several questions:

  1. Why did you name the option pool_id instead of more common name for name registration?
  2. poolboy uses high priority for the pool owner genserver process. Why didn’t you implement the same here?
  3. Why do you use Agent for monitor references storage. It seems that it is redundant and just decreases the performance of the whole pool (I am preparing a PR with change to plain map).
  4. Why do you use the Project.Private.Module naming schema? It is the first time I see something like this. It is strange because these Private modules are exposed in documentation which declares that they can be used by developer (which means opposite of Private)
  5. There’s a very bad bug with monitor_caller function which spawns a monitoring process for every run call what goes against the idea of having a pool of processes in the first places (since you end up spawning process for every call anyways). I’d suggesting moving caller monitoring to the handle_call get_idle_worker logic and not removing the monitor until the worker is released. Overall, following checkout/checkin pattern would solve the problem. Plus, these processes will live forever while the caller is alive. Consider some long-living process (for example very common pattern of GenServer which executes some command periodically) which calls this pool frequently. These monitoring processes would pile up until all memory is exhausted which is essentially a memory leak and will result in the whole BEAM shutting down.
  6. Poolex is not handling worker start errors and whole pool will die with MatchError if some extra worker fails to start. It makes this pool inapplicable for usage in environments where workers connect to external services (like databases or HTTP services which are most pooling use-cases) which can be unavailable or return 422 for example.
  7. Starting and stopping supervisor manually is a strange approach. I’d suggest to start one_for_all supervisor which has Poolex and Dynamic as it’s children instead of starting a Dynamic supervisor as a direct link to Poolex. This would work faster in application stop and would disable the unnecessary trap_exit flag

Also I’d like to mention that this approach of BusyWorkers and IdleWorkers module which manage the same structure is uncommon (I personally see it for the first time) but very nice to read and it makes it really easy to follow the algorithm.

I’ve found that you work in Авиасейлс and there’s a chance that you use this library in production. If you want an expert review of your other solutions, codebase and development practices to find more issues like I did just now, please leave me the message, my rates are low.

1 Like