Guard against race conditions in RegisterAlgorithm #62
Conversation
|
@qmuntal, can you fix the merge conflict from the other pr that just merged? |
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
Done! |
| @@ -161,10 +169,13 @@ func (a Algorithm) computeHash(data []byte) ([]byte, error) { | |||
| // set to 0, no hash is used for this algorithm. | |||
| // The parameter `hashFunc` is preferred in the case that the hash algorithm is not | |||
| // supported by the golang built-in crypto hashes. | |||
| // It is safe for concurrent use by multiple goroutines. | |||
There was a problem hiding this comment.
Changes looks good! Just a side question.
Should we not allow user to De-Register an External Algorithm and add a new one. I do not see any API like that?
There was a problem hiding this comment.
I'm not sure if we should allow deregistering algorithms. I would rather wait until someone ask for it, so we understand which is the use case.
| var ( | ||
| extAlgorithms map[Algorithm]extAlgorithm | ||
| extMu sync.RWMutex | ||
| ) |
There was a problem hiding this comment.
can we open another issue to account for this proposal?
There was a problem hiding this comment.
@qmuntal I think sync.Map is a better option than a map with a mutex.
I tend to avoid using sync.Map due to this comment in its docs:
The Map type is specialized. Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content.
But I agree that a sync.Map could also be used here and I wouldn't complain.
There was a problem hiding this comment.
Currently, there is no other invariants along with the map content. Well, it is a trade-off: readability and performance.
Using a plain map with a RWMutex is faster than sync.Map. Let's keep this PR unchanged.
NCC Group reported a race condition in
RegisterAlgorithm:This PR fix the race condition.
@SteveLasker @shizhMSFT