search

elixir-lang / gen_stage

Producer and consumer actors with back-pressure for Elixir
http://hexdocs.pm/gen_stage
1.52k stars 193 forks source link

Warn/fail when `ConsumerProducer` child spec has `permanent` restart policy #195

Closed hauleth closed 6 years ago

hauleth commented 6 years ago

This lead today to DoS in my application and I didn’t found mention about such case in docs. Official introduction into ConsumerProducer leave that to default value (which is permanent according to Supervisor spec).

josevalim commented 6 years ago

@hauleth why did you get a DoS? Because queues started to get full and never emptied?

hauleth commented 6 years ago

@josevalim permanent jobs will be restored regardless of the reason of the job exit reason, which mean that jobs that exited normally (because they finished their job, and aren't needed) are restarted by supervisor (because these are permanent), but due to the fact that their job is one time these jobs fails now with error, and are restarted again and whole process fails again, and that is happening until supervisor stop restarting due to reaching maximum restarts count. But while so these jobs keep occupying Ecto connections pool, which makes other processes to wait until these timeout and whole app goes down.

I think that in case of ConsumerProducer restart field should be required or it should defaults to transistent instead of permanent, as this would prevent restarting job after it exited normally.

josevalim commented 6 years ago

@fishcakez do we want to raise here, if permanent was given? Or maybe always warn?

fishcakez commented 6 years ago

@josevalim so if the child is permanent, the supervisor can only consume the first max_demand events from a subscription and processing each event must last "forever" or until terminate_child/2 is called. If a behavior like that is desired I think using manual with a GenStage process and a separate supervisor would be clearer and more explicit. Therefore I think we should raise to prevent error as there isn't a good use for permanent and permanent may trip people up.

josevalim commented 6 years ago

Fixed and release v0.13.1!