Propagate HTTPClient.Task<Response> failures to subsequent redirect tasks

[gregcotten]

Discussed in #753

No test yet, and would love advice on how to best make one...

[gregcotten]

Should redirectTask be stored weakly? Not confident about the ownership model here.

[Lukasa]

We avoid weak like the plague, so it should definitely be strongly held. I think in the idea solution this would form a singly-linked-list so the ownership won't be a problem, but I want to confirm that when I look at the patch.

[Lukasa]

Ok, I've taken a look.

First, I really like this patch @gregcotten. You've done a good job of threading this nicely through the code, and followed all the existing coding style. It's really rare to get contributors who do such a great job of that, so I wanted to express my appreciation.

Secondly, though, I wonder if we can make this simpler. I think the mere presence of a redirect task is sufficient to indicate that we need to cancel that task, as well as ourselves. So should we just unilaterally forward it on?

Relatedly, we need to ensure we only look at the redirect task while holding the lock.

[gregcotten]

First, I really like this patch @gregcotten. You've done a good job of threading this nicely through the code, and followed all the existing coding style. It's really rare to get contributors who do such a great job of that, so I wanted to express my appreciation.

Thanks so much! I'm trying my best to follow the style 🕵

Secondly, though, I wonder if we can make this simpler. I think the mere presence of a redirect task is sufficient to indicate that we need to cancel that task, as well as ourselves. So should we just unilaterally forward it on?

So remove FailAction.propagateCancellation and just cancel the redirect task if it exists?

Something as simple as this? The fail action was .none previously since the state was .finished, so that's why I wanted to handle it with the StateMachine.

private func fail0(_ error: Error) {
    self.task.eventLoop.assertInEventLoop()

    let action = self.state.fail(error)

    self.executeFailAction0(action)

    self.redirectTask?.cancel()
}

Relatedly, we need to ensure we only look at the redirect task while holding the lock.

Currently it's only being assigned when on task's event loop

self.task.eventLoop.assertInEventLoop()

[Lukasa]

Something as simple as this?

Yes, modulo the lock.

Currently it's only being assigned when on task's event loop

Right, but it's read from anywhere. So we need the lock.

[Lukasa]

As for the test, I think the right solution here is to rely on a httpbin endpoint with a long delay, plus a redirect. (The httpbin stuff is internal to the project)

You can use /redirect/target with the header X-Target-Redirect-URL set to where you want to go. That can redirect you to /wait, which will never complete, so you can use that as the trigger. Do the redirect, sleep for 500ms, then hit cancel. That should reliably work with your fix, and not reliably work without it.

[gregcotten]

Right, but it's read from anywhere. So we need the lock.

Actually re-reading this, I'm not sure I understand. I've made redirectTask a private var, like it's other mutable var state which is only accessed on the task's event loop (via self.task.eventLoop.assertInEventLoop() calls). When I call self.redirectTask.cancel(), that is also on the task event loop. All reads and writes of redirectTask occur on that event loop.

[Lukasa]

Oh I apologise, you're quite right. I am clearly losing my mind here.

[gregcotten]

OK, @Lukasa, ready!

[gregcotten]

Only last minute thought is that instead of simply propagating cancellation, we propagate the actual fail(...) Error

[Lukasa]

Yeah, let's propagate the fail.

[gregcotten]

OK @Lukasa, done and ready to go.

[Lukasa]

Awesome, I think this is looking really good.