New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
httpauth: Refactor out Mutex #69
httpauth: Refactor out Mutex #69
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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 |
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. |
Hi @cetra3 On master, this test fails with a panic.
On this branch it passes. |
031f2da
to
f85bd26
Compare
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 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. |
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? |
There is also another method, which skips a box pin allocation on the 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(())),
}
} |
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 |
If you return 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 |
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. |
f85bd26
to
8e969b6
Compare
8e969b6
to
bf96c2b
Compare
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. |
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 |
Tested on my end and it looks good ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved as per other comments
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. |
The two key changes are getting rid of the Mutex and having the call to `service.call` and the await of the future returned be on two different lines. Fixes "AuthenticationMiddleware called already" panic.
bf96c2b
to
f4b10d0
Compare
@robjtede I added in a comment. |
Fixes #10