avoid reauthentication loops#1746
Conversation
|
Build failed.
|
It does not make sense to reauthenticate multiple times for a single request. When the backing service returns 401 for a brand-new token, reauthenticating again will just send us into an infinite loop. Closes gophercloud#1745.
5a33db8 to
4b56f24
Compare
|
Okay, so this got way worse than I expected. The test failure is in I tried to replace the There is definitely something wrong with Reauthenticate(). I have some ideas and will poke around a bit. |
The old implementation has several places where the reauth mutex is unlocked before being locked again. This allows concurrent goroutines to glitch through reauthentication without the token having changed at all. Since mutexes and esp. RW mutexes are incredibly hard to get right, I propose this alternate implementation that coordinates concurrent Reauthenticate calls with channels.
|
Okay, so this PR also got way bigger than I expected. I threw away most of the mutexes in |
|
Build succeeded.
|
|
@majewsky Thanks so much for looking into this. I think I understand the implementation - it looks like you have implemented correctly what the removed code was trying to achieve. I can also confirm the existing tests are passing, even doing a count of 50 rounds. And Is it possible to add an additional unit test to reproduce the issue you discovered? |
|
I added a testcase. You can verify that the testcase detects the fixed bug by removing the |
|
Build succeeded.
|
jtopjian
left a comment
There was a problem hiding this comment.
LGTM. Let's give this a shot :)
It does not make sense to reauthenticate multiple times for a single request. When the backing service returns 401 for a brand-new token, reauthenticating again will just send us into an infinite loop.
Closes #1745.