httpauth: Refactor out Mutex

[boydjohnson]

Fixes #10

[codecov]

Codecov Report

Merging #69 into master will increase coverage by 1.76%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage   77.20%   78.97%   +1.76%     
==========================================
  Files          24       24              
  Lines        2005     2026      +21     
==========================================
+ Hits         1548     1600      +52     
+ Misses        457      426      -31     

Impacted Files	Coverage Δ
actix-web-httpauth/src/middleware.rs	75.00% <96.00%> (+75.00%)	⬆️
actix-web-httpauth/src/extractors/bearer.rs	10.52% <0.00%> (+10.52%)	⬆️
...x-web-httpauth/src/headers/authorization/header.rs	31.57% <0.00%> (+31.57%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d32248...f4b10d0. Read the comment docs.

[cetra3]

I'm a little worried that there hasn't been a repro for #10 and would be reluctant at introducing yet another mutex implementation without solid test cases. As you said, this is based on trial and error, and I think having a test case to prove that this is correct would be needed.

There appears to be some upstream issue that is causing try_lock() to return None, so we need to understand why that is happening first, as it is very much exceptional circumstances. The &mut self on both methods should not really allow for this to happen, so that needs to be sorted out why it's happening in the first place & may not even be related here.

[boydjohnson]

Thanks for the quick reply @cetra3 . I will work on some unit test cases for this situation. I'm going to first look at other tests for Service throughout the different actix codebases, to see if there is something useful there.

[boydjohnson]

Hi @cetra3 On master, this test fails with a panic.

test middleware::tests::test_middleware_mutex ... FAILED

failures:

---- middleware::tests::test_middleware_mutex stdout ----
thread 'middleware::tests::test_middleware_mutex' panicked at 'AuthenticationMiddleware was called already', actix-web-httpauth/src/middleware.rs:183:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Panic in Arbiter thread.

On this branch it passes.

[cetra3]

Thanks for your test, I can replicate the issue with the current master

However: If you use the same strategy you have, (polling the lock rather than panicking), the tests past without having to add a new dependency on future-parking_lot

I.e:

    fn poll_ready(&mut self, ctx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        match self
            .service
            .lock()
            .boxed_local()
            .as_mut()
            .poll(ctx)
            .map(|mut s| s.poll_ready(ctx))
        {
            Poll::Ready(inner) => inner,
            Poll::Pending => Poll::Pending,
        }
    }

Can you confirm if that works for your trial & error scenario as well.

[boydjohnson]

Hi @cetra3 I see that using the futures_utils::Mutex passes the test. It doesn't pass my trial and error approach which is to run a workload generator on an actix api with the Auth Middleware. I run the generator for 10 seconds and then run it again for 10 seconds. On the second try it no longer responds and does 0requests/sec which I am attributing to a deadlock. Do you suggest that I find a unit test that will show this failure?

[cetra3]

There is also another method, which skips a box pin allocation on the poll_ready method that appears to work on tests (but can result in false positives on poll_ready which is allowed according to API)

    fn poll_ready(&mut self, ctx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        match self.service.try_lock() {
            Some(mut guard) => guard.poll_ready(ctx),
            None => Poll::Ready(Ok(())),
        }
    }

[boydjohnson]

Hi @cetra3 That passes my trial and error test. I am hesitant since it doesn't make sense to me. Returning Poll::Ready(Ok(())) when the lock isn't available would tell the runtime to call call when the lock isn't ready. I tried with Poll::Pending but that doesn't pass the trial and error test. Can we talk that code through? Why did you choose Poll::Ready instead of Poll::Pending?

[cetra3]

If you return Poll:Pending from that second example it will not ever wake up as try_lock does not have the side effect of registering a waker as per lock(), so the task will never be notified. It is OK, but a bit ugly as you said, according to the API to return Poll:Ready (i.e, false positive).

I agree, I don't think it's finished code, or an acceptable permanent solution, but it is better than panicking or introducing a new dependency.

On a closer look at existing implementations, I think refactoring it to remove the Mutex altogether may be a viable solution. Having a look at some of the other middlewares in this package, none of them use a Mutex. There was a similar issue to this with the identity middleware: 1263 which may be a good starting point.

[boydjohnson]

I'll take a look at the identity middleware. Thanks for your advice. It may be a few days until I get back to this.

[boydjohnson]

Hi @cetra3 I ran a workload generator for 8 minutes twice on an actix-web api with this PR branch and there was no panic.

[cetra3]

That looks good. The next steps are we need to get the original issue raiser to see if your PR works for them & then get sign off from other members

[stiiifff]

Tested on my end and it looks good !

[cetra3]

Approved as per other comments

[robjtede]

The patch looks good. I think it would be worth documenting this nuance about locking Mutex/borrowing Refs across await points for future middleware makers. For now, can we add in just a line above the await pointing out the pitfall avoided.

[boydjohnson]

@robjtede I added in a comment.