bitcoin-dev-project / sim-ln

Payment activity generator for the lightning network
MIT License
63 stars 28 forks source link

refactor: use listen and trigger universally #164

Closed enigbe closed 8 months ago

enigbe commented 9 months ago

What this PR does

Related Issue(s)

Notes

carlaKC commented 9 months ago

Discussed this PR a little more offline. Another option for a cleaner/more testable solution would be:

This saves us from having to pass shutdown all the way down to every task, and cuts down on the number of places where we need to call trigger. Also has the benefit of making some of these functions more testable, because we can assert on return values.

Eg, for our simulation results task:

        tasks.spawn(async move {
            if let Err(e) =
                produce_simulation_results(nodes, output_receiver, results_sender, listener_results)
                    .await
            {
                shutdown.trigger();
                log::error!("produce simulation results exited with error: {e:?}.");
            }
        });
carlaKC commented 9 months ago

Can be rebased on #160 and sincerest apolgies in advance for all the rebase conflicts :')

enigbe commented 9 months ago

Discussed this PR a little more offline. Another option for a cleaner/more testable solution would be:

* Refactor to return `Result` from functions that can trigger shutdown

* Call `trigger` at spawn site rather than inside of the function

This saves us from having to pass shutdown all the way down to every task, and cuts down on the number of places where we need to call trigger. Also has the benefit of making some of these functions more testable, because we can assert on return values.

Eg, for our simulation results task:

        tasks.spawn(async move {
            if let Err(e) =
                produce_simulation_results(nodes, output_receiver, results_sender, listener_results)
                    .await
            {
                shutdown.trigger();
                log::error!("produce simulation results exited with error: {e:?}.");
            }
        });

I have refactored the handling of errors across all tasks. This reduces the number of places trigger() can be called and streamlines the logic. However, I think we can further reduce the call to trigger() to just one location.

From the snippet from sim-lib/lib.rs::run below, we await the completion of all tasks in the join set tasks. The error from the first task failure can be propagated until it gets here, in which we call trigger() (and break out of the loop) to shut down all listening tasks in the set.

        while let Some(res) = tasks.join_next().await {
            if let Err(e) = res {
                // log::error!("Task exited with error: {e}.");
                // success = false;
               self.shutdown();
               break;
            }
        }

This would mean we have to have another loop waiting for all tasks to exit (post trigger()) to have a graceful shutdown. Uncertain if this is a good idea/approach and happy to get thoughts on this.

carlaKC commented 8 months ago

Testing this and it looks like TrackPayment doesn't run anymore and the simulator hangs on shutdown.

Will take a look at the code, but iirc this is a regression since my last review (I tested last time and this was fine).

enigbe commented 8 months ago

That's weird. Taking another look right now.