[Part 6|*] Thread Contexts through payment methods Part 2

[ziggie1984]

Part 2 of the Context Threading refactor. Depends on #10307

[ellemouton]

nice! will give final ack once all dependent prs are in.

I think YY will have more context (no pun intended) around what is safe to do in payment lifecycle re context threading

[lightninglabs-deploy]

@ziggie1984, remember to re-request review from reviewers when ready

[yyforyongyu]

I think the current version is good enough, only have a question about the ctx used in reloadPayment, but that can be addressed in the final PR.

Re the ctx, in general for db write txns, we always want it to be finished. And for reads, we want it to be canceled if the parent ctx is canceled. We could rename cleanupCtx to be writeCtx to make it clear, or create a struct to hold two contexts - one from the caller, and one inherits the caller ctx which is used for writing. Tho I don't think it's urgent.

One last thing is what Elle has brought up before, that for the inherited context, we should add a timeout, just in case the db is locked, we need to funnel that error to the caller. But this is an open question, as we have a config for db timeout already, so I wander if we should use that here. There's also the question that, if the postgres is configured with a timeout, it seems unnecessary to add a timeout to the ctx as the call will return an error anyway?

[yyforyongyu]

similar to reloadInflightAttempts, we should also use ctx here to cancel the read operation?

[ziggie1984]

no we can't use the ctx context here, because we rely on this function recollect all INFLIGHT results, if we do use the other context we will basically abort as soon as we timeout.

[ziggie1984]

I tried to describe it in the todo where I introduced the new context:

	// TODO(ziggie): This is a workaround to avoid a greater refactor of the
	// payment lifecycle. We can currently not rely on the parent context
	// because this method is also collect the results of inflight HTLCs
	// after the context is cancelled. So we need to make sure we only use
	// the current context to stop creating new attempts but use this
	// cleanupCtx to do all the db operations.

[yyforyongyu]

We still have this downside such that, when the user cancels the operation, the path finding won't be stopped, but this is already a pre-existing issue, tho this will likely come up again when adding ctx to RequestRoute.

[ziggie1984]

agree, but luckily we are only talking probably of 1 sec waisted time and as described before we cannot fail this call because we depend on this for loop to collect potential inflight attempts for this payment.

[yyforyongyu]

I think this RPC is deprecated and will be removed, but guess we can remove after the PR is merged.

[ziggie1984]

yes let's do it in a separate PR.