Partition upload server

[NelsonVides]

If we're serving a ton of multipart requests in parallel, or we're having any other use of this temporary random file, that genserver singleton shows up in profiles, becoming a bottleneck. Thought it could be parallelised quite a bit by using a pool of them in a PartitionSupervisor, where the key is the owning process' pid.

[josevalim]

Instead of having a setup process, have a regular supervisor, do the init on the regular supervisor, and have PartitionSupervisor as the child of this new supervisor. :)

[NelsonVides]

Not used to doing any init work on supervisors because OTP live upgrades would scream if the supervisor init callback has side-effects. I know that generally Elixir doesn't encourage good-old-live-code-upgrades but still, the habit is very strong 👀

[NelsonVides]

Also, a supervisor doesn't give me a terminate callback to run where I can clean up the tables. I guess the workers could do it, and race between them like crazy, it's all idempotent 🤷🏽‍♂️

[josevalim]

let's avoid these changes as part of this PR for now, as they are unrelated?

[NelsonVides]

Fair enough 👌🏽

[NelsonVides]

@josevalim pushed a change according to your comments. Will cleanup those dummy timestamps in a separate follow-up PR 👍🏽

[NelsonVides]

Just noticed CI

Generated plug app
14:56:00.750 [info] Application plug exited: Plug.Application.start(:normal, []) returned an error: shutdown: failed to start child: Plug.Upload.Supervisor
** (EXIT) an exception was raised:
** (ArgumentError) errors were found at the given arguments:

• 2nd argument: invalid options
  (stdlib 3.17.2.4) :ets.new(Plug.Upload.Dir, [:set, :public, :named_table, {:read_concurrency, true}, {:write_concurrency, :auto}])

write_concurrency: :auto requires OTP25, didn't know we were supporting here such older <24 versions 🤔

[josevalim]

Check the OTP version and swap to auto if >= "26" (I think it was added on 25.1 or something)

[josevalim]

Also, can you please rebase? I found we were not covering a path, so I pushed new tests (and they should still pass on your branch).

[NelsonVides]

Rebased on top of the new master, and added the check for the OTP version. auto is available since 25.0 (https://www.erlang.org/doc/apps/stdlib/ets.html#new/2 and https://github.com/erlang/otp/blob/4ed7957623e5ccbd420a09a506bd6bc9930fe93c/lib/stdlib/src/ets.erl#L317) :)

[NelsonVides]

Oh, and added back trapping exits into the genserver, as we do want its terminate function to run on shutdown 😄

[josevalim]

@NelsonVides the terminate function means gen servers can race each other. Instead, add a Plug.Upload.Terminator before the PartitionSupervisor that simply traps exits and deletes everything on terminate. :)

[NelsonVides]

But that's what the Setup process was doing, if we're already creating such ad-hoc helper process, don't we want to also move the initialisation (creating the ets tables and so on) into it, to at least ensure we don't make live upgrades any harder than they already are?

[NelsonVides]

Also, having a look at CI, PartitionSupervisor was actually introduced only in v1.14.0 (https://hexdocs.pm/elixir/main/PartitionSupervisor.html) 👀 So... isn't that only the last five minors are supported and therefore we could technically raise the minimum version to 1.14? Or do we need some compile-time magic to distinguish which version we prefer?

[josevalim]

The reason why I disliked setup is because it was creating ETS tables that was used by its siblings. I prefer to keep it within the hierarchy. Ideally, if supervisors had a terminate, we would terminate there, but since we don’t, we need the additional process only for termination!

regarding the Elixir version, let’s require 1.14 and bump CI!

[NelsonVides]

All right, pushed a new version, CI bump included 🤞🏽

[josevalim]

💚 💙 💜 💛 ❤️