refactor(DetailedStopGroup): replace async_stream

[thecristen]

Scope

MBTA-DOTCOM-JV

In the continued vein of, we unearth old but recurring Sentry issues from the archive...

Uncaught exit - {:timeout, {Task.Supervised, :stream, [5000]}}
    lib/task/supervised.ex:349: Elixir.Task.Supervised.Task.Supervised.stream_reduce/7
    lib/enum.ex:4570: Elixir.Enum.Enum.flat_map/2
    lib/dotcom_web/controllers/stop_controller.ex:269: Elixir.DotcomWeb.StopController.DotcomWeb.StopController.get_stop_info/0
    lib/dotcom_web/controllers/stop_controller.ex:44: Elixir.DotcomWeb.StopController.DotcomWeb.StopController.show/2
    lib/dotcom_web/controllers/stop_controller.ex:1: Elixir.DotcomWeb.StopController.DotcomWeb.StopController.action/2
    lib/dotcom_web/controllers/stop_controller.ex:1: Elixir.DotcomWeb.StopController.DotcomWeb.StopController.phoenix_controller_pipeline/2
    lib/phoenix/router.ex:416: Elixir.Phoenix.Router.Phoenix.Router.__call__/5
    deps/plug/lib/plug/error_handler.ex:80: Elixir.DotcomWeb.Router.DotcomWeb.Router.call/2

Implementation

This message appears when Task.async_stream doesn't specify an on_timeout action - the default behavior is the task exits!

In this case I think it's better if we don't create possible timeouts: just get everything anyway without async calls, since we'll always want this page (/stops) to list all the stops.

I think the code had likely been structured this way because fetching and processing all the stops can be kinda slow. So this updated code leverages Stream over Enum in a few spots for some hopeful performance improvements. The flame charts are still massive here, but I did see this page go from producing an SVG of 5.2MB to one that's 3.4MB...

Before	After

[joshlarson]

I'm on the fence about this change, tbh. The stations page does in fact load stations for every non-bus route before rendering (even if you're just looking at the subway stations tab), which means that now we're making 29 @stops_repo.by_route/2 calls in sequence, instead of in parallel.

In practice, it's probably okay - I tested it out and while it does seem to increase page load time when the cache is empty, that's likely to be a once-or-twice-per-day sort of deal.

I feel like it would reasonably be equally correct to either have these async_start calls handle timeouts more gracefully (retry?) and/or to have timeouts bubble up and cause the whole page to 500.

[thecristen]

✌🏼 Take two: restore the async_stream calls but make it a little more sane by:

• Not nesting so many of them within each other (!)
• bumping up the max_concurrency value
• using the on_timeout: :kill_task argument (eh, that'll mostly just give us a nicer error message)

Behavior stays the same as before: a task timeout will ultimately make the page return a 500. But I hope to make that happen less frequently by avoiding these functions from timing out.