Actix-web: Should we drop failure?

[fafhrd91]

actix wont support Send, failure enforce everyone to use Send + Sync

should we just use our own Fail trait

[mitsuhiko]

I don't think that is going to work. There are more and more failures flying around and I think it will be almost impossible to lock out that ecosystem. In which situations does this causes issues currently?

[fafhrd91]

We have InternalError(HttpResponse). And HttpResponse is not Send

[mitsuhiko]

I think the best approach could be to have things like InternalError be a wrapper around (*mut HttpResponse, ThreadId) and only give access to the HttpResponse if it's from the same thread. I do something similar currently in our error reporting library. Then an unsafe fn can be provided to get the interior type in places where we know it's safe to do to bypass the thread id check.

[mitsuhiko]

On second though ThreadId probably has a not insignificant chance of being reused. This might not be good enough. Need to contemplate about this more. But something like that could work.

[DoumanAsh]

I wouldn't mind to drop failure it seems to be unnecessary 🤔

[mitsuhiko]

@DoumanAsh it would be a massive problem for our project. We're migrating all our libraries from std::error to failure as without that errors in production in complex projects are almost impossible to pin down. It's unlikely that failure will lose the Send bound in particular now that futures and tokio is moving towards Send.

[xfix]

I wonder if it's possible to have a thread safe version of HttpResponse, created when a failure is needed?

[mitsuhiko]

In actix at least there is never a good reason to move an HttpResponse object into another thread so this is optimizing for a case that should never come up. If we can find a way to anchor an object into a thread safely that this would solve those issues.

In general while this behavior is obviously not sound right now, as far as behavioral issues go this seems pretty small and it makes sense to focus on other issues first. I'm pretty sure the vast majority of usages of error handling are unlikely to experience any unsafe behavior here.

[fafhrd91]

I think there should be no ub here, but for purity reasons we can drop failure and make adapters for it.

[seanmonstar]

Note that moving a !Send value to another thread is almost always UB, even if trying to guard with the ThreadId. The problem is the destructor: even if you never access any of it's memory in the other thread, when it is dropped, the destructor can mutate the memory without synchronization.

That could happen when the "guard" drops naturally, or if the thread closes naturally (or because of a panic).

Perhaps in Drop of such a guard, you could force the value to leak if not on the same thread, which is better than UB, but it's probably better to just not be Send.

[mitsuhiko]

The thinking is that the guard would have to leak if it was in a different thread. Yes.

[seanmonstar]

To be clear, such a guard would need to call mem::forget.

struct LeakyGuard {
    val: Option<Val>,
    id: usize,
}

impl Drop for LeakyGuard {
    fn drop(&mut self) {
        if !guard(self.id) {
            if let Some(val) = self.val.take() {
                ::std::mem::forget(val);
            }
        }
    }
}

However, doing so with the ThreadId is still able to trigger UB, since the original thread could have shutdown, and its ID can be reused.

[mitsuhiko]

@seanmonstar yes. i have the same issue elsewhere so i will try to put together a crate that has a sane implementation of such a container and then see how it goes. In another project with a similar project i use just the threadid but i don't like that this is still UB. But a counter stored in TLS might be a good solution.

[seanmonstar]

As it pertains to Actix, it seems the only concern is that InternalError holds InnerHttpResponse, which has an unsafe impl of Send and Sync. A possible solution to remove that unsafety is to create a new type, InnerSendHttpResponse (sorry, naming is hard), and move over any fields that should be kept (so, ditch the Body, but hold an Option<Binary>).

[mitsuhiko]

FWIW my interest in this topic is twofold. I'm a strong proponent of retaining the Sync and Send traits in failure and have the ecosystem standardize on failure but have a convenient story for people that cannot fulfill this bounds. So I want to see this particular issue also in the context of failure itself.

[seanmonstar]

You could possibly try something by putting the value in TLS, and passing a key instead. Then, the value never leaves the thread, and it's much easier to reason about whether you were allowed to access the value: if you tried on a different thread, your attempt to access the slot will fail, for obvious reasons.

That key could be something like a u64 generated from a static AtomicU64 or something... It could overflow after some quintillion uses, but :shrug:. However, it runs the risk of map leak, especially if they're generated on the main thread.

It's quite tricky, and probably the better solution to propose to people is: if you want to create a Fail from something that isn't thread-safe, you should just make safe owned copies of the data that is important for the error.

[mitsuhiko]

I committed a wrapper type here I'm reasonably happy with: https://github.com/mitsuhiko/rust-fragile/blob/master/src/lib.rs

[fafhrd91]

Best solution would be to have failure::Fail without Send + Sync and use send sync on failure::Error

[fafhrd91]

btw what is the reason for Sync+Send for Fail trait?

i fixed unsafe impl for InnerHttpResponse, we are good for now.