HTTPDigestAuth - Making it thread-safe

The existing code counts the number of 401 responses in the num_401_calls
authenticator attribute. This is in place so as to ensure the necessary auth
header is sent, while avoiding infinite 401 loops (issue #547).

This commit makes num_401_calls an instance of threading.local() (previously
an integer), using num_401_calls.value as the counter.

It ensures that concurrent authentication requests get each their own counter
and behave as expected (otherwise every other concurrent request would have
its authentication fail).


@Lukasa , @kennethreitz - Per your suggestion, I just updated the PR with commit e65360d.
Thanks for your input.


I think the code is racy in nearly all the areas, not just the 401 counter.
- build_digest_header is updating a number of internal variables including nonce lastnonce, etc.
- if there are several unauthenticated request at the same time, you will get several challenges, but stored in the same variables for subsequent request.

I think a more stable approach would be to store the whole state in a per thread variable. This has the drawback of doing one auth per thread, instead of trying to reuse the chalenge data from other threads.


@tardyp : You are correct. This needs further improvement, just as you point out there are other attributes currently shared accross threads that will lead to auth failures in certain conditions.

I'll extend the per-thread approach of num_401_calls to the remaining state-tracking attributes and update the PR for review.

Regarding the drawback you refer to I think it comes as a reasonable compromise:
- This approach won't hurt the currently working single-thread case.
- It guarantees correct authentication in a multi-threaded case (which requests claims to support).
- Simple and easy to understand code change.


@exvito : @vincentxb and I are on the same team, and have story on our sprint to resolve this issue. I was working on it this morning, and am at the point of creating a unit test.


Updated with commit e8d9bc5. Highlights:
- All state now in thread local storage.
- Factored out state initialization to `init_per_thread_state()` which must be called from `__init__()` for the regular, single-threaded case and, eventually, from `__call__()` in the cases where threads that did not create the auth handler invoke it.
- Maybe the `self.tl` name can be better: I opted for short, project leaders may prefer `self.thread_local` or some variation.
- Maybe the test for thread local state initialization in `__call__()` can take a different approach.

Here's a contribution. Thanks for your feedback.
PS: @tardyp does that mean you'll work from this PR or you'll take a different approach and suggest I drop this PR?


This looks entirely reasonable to me. It would be ideal to have some tests for this if at all possible.


@exvito, we worked on the same approach at the same time. 

I opted for the name thd for the local variable, and did the same ini_per_thread call. I think it is not necessery in **init** however, only in _call_

 :+1:  

I have a unit test on a separate commit:

https://github.com/tardyp/requests/commit/e15ed2c46823183b73250503fc44146f979eb891

Please cherry-pick it!


> I think it is not necessery in `__init__` however, only in `__call__`.

You are correct. Additionally, after taking a peek at your code, I opted for your approach of ensuring per-thread state is initialized once and only once based on the `self.tl.init` attribute.

Updating with two commits:
- Your cherry picked test case (confirmed to work nicely, thanks).
- A simpler approach at initializing per-thread state.


I still like this idea, but I have some notes in the diff. =)


Is this code spawning a new thread?


@kennethreitz The unit test for it is, but the product code doesn't. It's just to make it safe when it's called from multiple threads.


:+1: