Making a library with Supervisor + GenServers: can't decide what's more convenient to use

Hi @dimitarvp - you would be in good company with either option - I’ve just looked through the app supervision trees of 3 or 4 apps and it appears reasonably evenly split - e.g. Ecto.Repo uses your option 1, whereas Phoenix.PubSub uses your option 2.

I prefer the clarity in the supervision tree of Option 1, but…

One advantage of Option 2 is less indirection - it’s easy to take a look into the entry points of your library and see what it is up to. Macro-based code can take a bit more effort to grok if there are issues, depending on what macro-fu moves you pull.

Also, Cachex is probably a good comparison given what it does. It uses Option 2. If your :name option is any atom (rather than a module), that could make your examples a bit cleaner, e.g.:

MyLibrary.each(:pool1, items, function)

instead of

MyLibrary.each(YourApp.YourWorkerPool, items, function)

Option 2 makes it a little easier to play with in iex:

  {:ok, pid} = MyLibrary.start_link(name: :test_pool, workers: 15)
  MyLibrary.each(:test_pool, 1..200, &{IO.inspect({self(), &1})})
  GenServer.stop(pid)  

Another point… libraries that use use do typically add behaviour (e.g. Broadway) or complex configuration that has to “stay alive” (e.g. Ecto.Repo) in MyModule, whereas in your case, the vast majority of the application-specific complex behaviour is externalised into items and function and only introduced into your scope for the purpose of multiplexing the calls, so IMO there is little value in having MyModule

It’s a good question, and no easy answer!

Option 2, 100%. The less macros there are in the Elixir ecosystem, the better. use makes it difficult to know what is going on with the module and makes compilation slower due to more compile dependencies. New users of Elixir already have it difficult enough with all the various libraries that have their macro tricks.

The latter is the “standard” way how things should work and it’s more readily understandable what is going on: clearly we are using lib MyLibrary to run something, instead of this magic module. If the user so wants, they can easily write

defmodule MyPool do
  def each(items, function) do
    MyLibrary.each(MyWorkerPool, items, function)
  end
end

which again stays clear as to what is going on vs. a use statement (who knows what code is generated? how can I ensure I don’t accidentally make a function that conflicts with a generated one?).

If you want clarity in the supervision tree, you can in a similar vein write a function and use it in the list: children = [..., MyPool.child_spec(), ...].

The point is, option 2 leaves these tools available for the dev but doesn’t force them. As a dev I appreciate when I can make my own choices. The documentation can then provide useful copy-paste examples for novices.

One additional thing, which may not appear so useful here but might be more useful in other libraries: option 2 allows for runtime selection of the pool to use, whereas option 1 doesn’t. If this was needed the dev would need to use the option 2 form anyway, which would be non-obvious to discover and would mean they have two different calling conventions used.

Hm, excellent point about reducing app supervision tree visual bloat by using child_spec. I completely forgot about it and I’ll add it, thanks!

Plenty of good thoughts. The “don’t use macros unless you absolutely have to” resonates strongly with me.

I came here to say something similar! I like the idea that option 2 allows me to create an abstraction if I want to de-clutter or hide details but I don’t have to, whereas option 1 doesn’t really give me the choice.

I think I often favor libraries that present collections of composable things because I can choose the level of abstraction I want.

What a fun question to ponder, I liked that you asked this @dimitarvp, I recently found myself in a similar bit of analysis paralysis myself

I’m striving to figure out the purpose. Are you using this to limit the amount of concurrent operations in your system?

Either way, my vote definitely goes for option 2, because it’s the simpler version that perfectly serves your purposes. The user can (but doesn’t have to!) encapsulate this into a separate module.

I find option one overly complex, and in general I cringe when the abstraction needlessly has to be used. The use form is very implicit and IMO can and should be avoided in most cases. A good example where use is fine would be a behaviour with default callback impls, which is not the case you have here

Yes. I didn’t mention it will have a non-blocking version as well. And yep, it’s meant to prevent bursty memory usage since occasionally there are many new tasks to run (on a schedule). And the code is deployed on fairly small instances – think 2 vCPU and 256MB RAM. And there’s a lot of binary wrangling.

…Thinking of it, I probably should have just went with Oban and limit the load there, and call it a day.

Still, it was a very fun exercise that helped me cement various OTP knowledge pieces and I’ll open-source this micro library at one point. I know at least two companies that will be happy to use it.

For sync operations you could use jobs. Here’s an example of a rate limited queue. You basically need to replace the {:standard_rate, 1} option with something like {:counter, {:limit, n}}, and it becomes a concurrent queue (it allows at most n operations at the time). See docs for details.

With jobs, the enqueued operation is running in your process, not inside some worker process, which eliminates the need to copy the data to another process in sync scenarios. It also means that multiple unrelated jobs won’t run in the same process, which I think is a better approach to having a pool of long running workers.

For async, however, you’ll need to spawn a separate process (probably a task) and invoke :jobs.run from there.

You may also be able to use GenStage in the front with the ConsumerSupervisor at the back.

Although I am wondering if we shouldn’t add this to DynamicSupervisor. We already support max_children. Making it buffer would be relatively straightforward.

Guess I was turned off because it was Erlang and I am able to read it as quickly as Elixir. Thanks for the pointer, will study it!

Not sure I follow. It’s the entire idea of a job queue having N child processes chugging away at jobs, no? If we’re going to work in the same process then we might as well just use Enum.map. I am likely misunderstanding, apologies.

Unless there are suspicions of leftover big binaries that can’t be GC-ed then IMO persistent workers are OK. But then again, we always get caught by surprise by those non-GC-able binaries so I see your point.

There’s also opq btw. It uses throwaway workers.

Cont’d in a reply to Jose below.

Ha, could you elaborate on how would this look, please? Very interested that it didn’t even occur to me!

Alternative scenario to consider (/cc @sasajuric). I had one workflow where I needed two layers of workers, not one:

1. Callers. They do GenServer.call addressing the pooled workers. This blocks so it’s best done in a separate process.
2. Actual workers. They basically receive a GenServer message containing function and an arg and run the function with the arg. But since we can have e.g. 10 items and 3 workers then the callers seemed like a useful construct since in that case we’d spawn 10 callers that will all wait on GenServer.call, each addressing 1 out of 3 possible PIDs.

Now have in mind that was no less than 5.5 years ago and I didn’t understood OTP that well back then so it was likely over-engineered. But my idea at the time was to write something of my own so I can fully grasp how OTP works – it served that purpose but it might be useless otherwise.

(And I’ll openly admit I am always intimidated by GenStage… That’s very likely a mistake.)