cmcfarlen commented on PR #13616: URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5516184036
Correct as stated, but deliberately out of scope here. `allocated()` reads `_blobs[blob]`, `_cur_blob` and `_cur_off` without the mutex because that is what every other id-taking accessor in this class already does: - `Storage::valid()` — the same two fields, unlocked - `Storage::lookup(IdType, ...)` — `!blob || (blob_ix == _cur_blob && offset > _cur_off)`, unlocked - `Storage::name(IdType)` — the same guard, unlocked - `Storage::rename()` — the same guard, before it takes the lock So this is not a race introduced by unlisting; it is the existing synchronization model of `Storage`, and `allocated()` was written to match it rather than to invent a second convention in the same class. Making just this one function lock while its neighbours do not would be misleading about the guarantees, and taking `_mutex` in `allocated()` would also put a lock in the iterator's skip path, which is currently lock free by design. The synchronization of this class is being addressed directly in #13583, "Metrics: close the id lookup race and bounds gaps left by the lock revert". That is the right place for it — it is a property of the whole store, not of this feature, and fixing it in two PRs at once would just produce conflicts. Whichever of the two lands second should extend the fix to cover the other's accessors: if #13583 goes first, `allocated()` needs the same treatment; if this one goes first, #13583 picks it up along with `valid()`, `lookup()` and `name()`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
