saml
saml copied to clipboard
Fix IDP deadlock due to recursive locking
While using the IDP server for integration tests, I've experienced a deadlock after issuing many concurrent requests to the following endpoints:
-
PUT /services/:id -
GET /sso
Apparently, the issue is that the IDP code attempts to acquires locks on the Server.idpConfigMu recursively:
- The /sso HTTP handler calls
Server.idpConfigMu.RLock() - Before releasing the above lock, this same handler eventually calls
s.GetServiceProvider(), which in turn attempts to acquire the same lock by callingServer.idpConfigMu.RLock() - Concurrent requests to
PUT /services/:idacquire a write lock by callingServer.idpConfigMu.Lock()
Step (2) is a recursive lock, which won't work, as stated by the documentation of sync.RWLock[1]:
If a goroutine holds a RWMutex for reading and another goroutine
might call Lock, no goroutine should expect to be able to acquire a
read lock until the initial read lock is released. In particular,
this prohibits recursive read locking. This is to ensure that the
lock eventually becomes available; a blocked Lock call excludes new
readers from acquiring the lock.
This patch fixes the issue by removing the unnecessary lock acquired in step (1). This was redundant since GetServiceProvider() already takes care of serializing access to the s.serviceProviders map, which is the resource in question.
[1] https://golang.org/pkg/sync/#RWMutex