Poolex - A library for managing pools of workers

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.

Hi, @Asd!
(@hst337 ? )

First of all, thank you very much for reading the code and describing several problems you found! Writing a project without a code review was quite tricky, and I no longer saw the issues you wrote about.

Why did you name the option pool_id instead of more common name for name registration?

Initially, I used only atom() as the first parameter since I did not see the need to complicate it. I hadn’t thought about using Registry, and it turns out that in my years of working on Elixir, I’ve never had to use anything other than atoms. In general, when a pool was intended to have a unique atom as its identifier, pool_id was an appropriate name. Most likely, I made a mistake by not changing the name of this option when supporting the GenServer.name() type. But I don’t see any great criticality in this. I will make an alias :name, deprecate the old key :pool_id, and then slowly migrate it.

poolboy uses high priority for the pool owner genserver process. Why didn’t you implement the same here?

Unfortunately, I didn’t understand what we were talking about. Can you please describe it in more detail?

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).

You’re right! Thank you! It seems to me that there was a similar reason in one of the previous implementations because not in all cases, when working with monitoring, I had a State available. It looks like there are no more reasons, but I forgot to check this and remove the public storage for monitoring.

I’ll be waiting for your PR

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).

These new processes are very lightweight and not linked to the caller process. In any case, we need to monitor the caller, and I don’t yet understand why a process that waits to see if the caller will crash is not suitable for this task.

Overall, following checkout/checkin pattern would solve the problem.

It seems that this is excessive control over the execution logic on the call side. When using a pool, you should just perform some operation on it instead of juggling worker processes. What do you think?

Plus, these processes will live forever while the caller is alive.

I’m afraid that’s not right. The monitoring process is always killed after the worker is released.

Pay attention to this line: poolex/lib/poolex.ex at develop · general-CbIC/poolex · GitHub

Please tell me where I’m wrong if I don’t see something.

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.

This is an excellent observation. Thank you very much! I focused on carefully handling errors from an already running worker and forgot about controlling their launch.

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.

I didn’t understand
Why is this behavior strange, and how is it conceptually different from general supervisory behavior?

I missed this question. As far as I know, Elixir does not have private modules, and there is no way to limit module accessibility. I added the Private space to clarify to the developer that he is doing something wrong.

First, the difference in application stop. When supervisor will stop Poolex process, this process will have some time to handle some messages and then terminate (if it has a trap_exit flag) and only then the DynamicSupervisor will start terminating. Usually, Supervisors give some time for children to terminate and then, when some child is terminating for too long, Supervisor kills it. With spawning link to DynamicSupervisor directly, it would have no such timeout and the Supervisor of Poolex won’t even know that there’s some DynamicSupervisor which is terminating, therefore DynamicSupervisor won’t have time to terminate workers. If these workers were HTTP connections acceptors, they would just die, while they could terminate gracefully with controllable timeout.

Second, the difference in fault tolerance. Right now if Poolex dies, all workers die with it. However, it is possible to just restart Poolex without restarting workers if dynamic supervisor and Poolex were started under the rest_for_one supervisor. Poolex would then just initialize it’s state and monitors from which_children of DynamicSupervisor.

And you can monitor it in the Poolex process without spawning extra one. This is what this checkout/checkin pattern is about: you store an association between a caller and a worker once worker is found and when caller or worker dies, you release the alive one.

I was talking about high priority for the dispatcher/manager process (Poolex process in your case) so that it checks out processes faster, since it manages the queue of work on it’s own and there’s no need to keep the messages in the messagebox. I thought that poolboy uses it, but it does not, while a lot of other pools with similar architecture do (like hackney, lhttpc). But it would still make sense to use it in Poolex

True, I am wrong, I missed this line completely.

That’s another user, not me.

But developer can use the Private.DebugInfo and Private.Metrics modules, right?

And I also found one more bug. Consider this scenario:
Caller gets a worker, sends a long-running job to the worker, then caller dies and Poolex just returns this worker to the idle queue. In this case the next caller, can receive this worker which is still executing the long-running job and this caller won’t be able to execute anything with this worker. So I have a feeling that it might make sense to just restart the worker when caller dies before releasing this worker

LOL, I miss @hst337.

The “official” way to mark a module private is to add @moduledoc false, that way they don’t show up in the documentation (I can find a citation if needed). Looking through your Private namespace none of the moduledocs are very involved and could just be comments. Though I don’t think it’s a big deal either way, just pointing it out. Earmark uses a similar approach with Earmark.Internal.

If I understand correctly, you propose changing the circuit, as shown in the figure below. It seemed that if the Poolex process receives an EXIT message, it starts executing the terminate() callback after a short time. The first step in this callback is to turn off DynamicSupervisor: poolex/lib/poolex.ex at develop · general-CbIC/poolex · GitHub.

On the Application Supervisor side, both trees are represented through a single input process: Poolex or Poolex Supervisor. I think that killing the input process will result in the same termination logic in both trees.

I completely agree with this. However, it is unclear where you can dump the state with information about which workers are busy and which are not.

image781×859 18.7 KB

image948×849 22.5 KB

I need to think about it.

Oh, cool! I will check this out

Yep. And I can’t do anything with it. I can only say that it is for internal library usage. I love Elixir, but unfortunately, it doesn’t have a feature to hide library interfaces.

Thank you very much! I will add it to issues to not forget

However, I want to document private modules and functions for “future-me” or other Poolex contributors. Hexdocs is not all because developers still get access to each module’s public interfaces even if they aren’t documented.

you store an association between a caller and a worker once

I learned recently that it might not be necessary to store the associations in the monitoring process since we can provide tags to :erlang.monitor which would be included in the DOWN message. And since the caller gives ref and resource back on checkin, we don’t need lookups there either.

def checkout(pool, callback, timeout) do
  {ref, resource} = GenServer.call(pool, :out, timeout)

  try do
    callback.(resource)
  after
    GenServer.cast(pool, {:in, ref, resource})
  end
end

# assume no queue
def handle_call(:out, {_ref, pid}, state) do
  # ... queue handling and stuff ...
  [resource | resources] = state.resources
  ref = :erlang.monitor(pid, tag: {:DOWN, resource})
  {:reply, {ref, resource}, %{state | resources: resources}}
end

def handle_cast({:in, ref, resource}, state) do
  # ...
  Process.demonitor(ref, [:flush])
  {:noreply, %{state | resources: [resource | state.resources]}}
end

def handle_info({{:DOWN, resouce}, _ref, ...}, state) do
  # ...
  {:noreply, %{state | resources: [resource | state.resources]}}
end

Disclaimer: I didn’t read the whole thread or the Poolex code, but thought this little bit about monitors might be useful Sorry if it’s completely irrelevant to the discussion!

And maybe cleanups like GenServer.cast(pool_id, {:cancel_waiting, caller_reference}) can be replaced with process aliases (available since OTP-24+, Elixir 1.15+) and the caller queue can maybe be replaced with just the message queue! Reading the docs a bit more carefully, it might not work. I’ll need to try out in a project tomorrow.

Either way, it’d be really cool if Poolex used all the modern features of the Erlang VM! Then it would be another good reason to use it over Poolboy

Hi, @ruslandoga!

Thanks a lot for the tip! At first glance, with the help of tags, we can refuse to store monitoring links to the “types of process” we monitor (worker or waiting caller). I’ll have to check it out.