Hi, nice library, I’ve read the code and I have several questions:
- Why did you name the option
pool_idinstead of more commonnamefor name registration? poolboyuses high priority for the pool owner genserver process. Why didn’t you implement the same here?- Why do you use
Agentfor 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). - Why do you use the
Project.Private.Modulenaming schema? It is the first time I see something like this. It is strange because thesePrivatemodules are exposed in documentation which declares that they can be used by developer (which means opposite ofPrivate) - There’s a very bad bug with
monitor_callerfunction which spawns a monitoring process for everyruncall 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 thehandle_call get_idle_workerlogic 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. - Poolex is not handling worker start errors and whole pool will die with
MatchErrorif 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. - 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_exitflag
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.
)
