| Summary: | CheckedLock::tryLock() use pattern should be more robust | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kimmo Kinnunen <kkinnunen> | ||||
| Component: | Web Template Framework | Assignee: | Kimmo Kinnunen <kkinnunen> | ||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Local Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=224970 | ||||||
| Attachments: |
|
||||||
|
Description
Kimmo Kinnunen
2021-04-30 05:55:53 PDT
Bug 224970 (In reply to Chris Dumez from comment #15) > > My preference would have been: > TryLocker locker(lock); > if (!locker.isLocked()) > return; > > This is also what Blink uses. > > I dislike any kind of adopted lock. I don't think we should be interacting > with the lock directly or adopting lock as this is a recipe for mistakes. Blink MutexTryLocker does not support thread safety analysis. As such it is equivalent of already implemented auto locker = tryHoldLock(). https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/threading_primitives.h I failed to write a TryLocker that would support current implementation of thread safety analysis. Suppose we set aside the preferences for syntactical constructs for some time. The decision of what syntax to use wrt tryLock is based on which problem are you solving: A) The problem to solve is that caller forgets to adopt the lock after tryLock(). B) The problem to solve is that caller accesses the locked variables without lock. My (relatively small) experience leads to think that B is the more important problem to solve. In patch below I'm trying to communicate that the thread safety analysis already covers all of B and most of A. Created attachment 427412 [details]
Patch
|