We had a race condition that could first display the report and then
replace it again with the "loading" spinner. That doesn't seem to happen
now that we changed the order of cable events.
Rubocop was complaining about too many arguments. But
`ApplicationJob#perform` needs all arguments handled in one call. While
we could allow the `perform` method generally to have more arguments,
there could be other methods called `perform` which should still be
scrutinised. Instead, it seems acceptable to me to have more arguments
as long as they are clearly named as keyword arguments. Rails uses this
a lot to document all options including their default values, for
example in Active Storage. It's better then bundling several arguments
in an undocumented hash just to reduce the number of given arguments.
And once we upgraded to Ruby 3.1, we can clean the method calls up as
well. `call(user: user)` becomes `call(user:)` without repetition.
This is not a normal pattern for setting up ActionCable channels, so it might need some notes. It ensures the broadcasts from the ReportJob are unique not just to the user session but also to the specific tab in the user's browser. Otherwise if the user has two different report pages open in separate tabs with the same session, the broadcast would overwrite the #report-table element in both of them.
Forking worked in theory but crashed the browser in system specs. It
also came with many other hurdles and isn't well known solution in the
Rails community. Sidekiq can give us better control over execution
limits as well.
Sidekiq doesn't have any features to limit memory usage or execution
time. We need a separate process for this. Forking avoids the boot time
of a fresh process and copy-on-write ensures minimal memory overheads.
This is supposed to lower the memory footprint of all Puma workers. The
reports code will occupy needed memory in one Sidekiq worker instead of
in several Puma processes.
The current code doesn't limit the execution time yet. We either need a
way to terminate the report rendering after a while or send an email
with a link to access a rendered report.
The ReportsController was referencing the report and the report was
referencing the controller. It's unlikely that this circular dependency
created a memory leak but it's generally a bad design. And we need to
make the reporting independent of the controller to isolate it in a
background job.
