-
Notifications
You must be signed in to change notification settings - Fork 32
Description
@banasiak and I were looking over the Kotlin code's usage of @get:Synchronized
yesterday and concluded that a majority of it is actually unnecessary. X509TrustManagerExtensions
and friends are in fact thread safe and don't introduce mutable state that may be clobbered or result in exceptions. We concluded this by reviewing both the class implementations themselves and by looking at popular Android libraries like okhttp
that utilize the same server trust methods we do from a concurrency-supporting network client.
At the time I wrote the associated Kotlin code, I was referencing Chromium's Java implementation which wrapped everything in essentially mutexes. I incorrectly assumed it was because of a limitation in the platform's implementation of those classes, when it actually was just their support for "hot reloading" trust anchors as they change AFAICT.
Given that rustls-platform-verifier
doesn't do any kind of reloading today, we should be able to get rid of these locks without any harmful side effects. This should optimize the verifier's performance in multi-threaded Tokio apps (such as 1Password's) and let network requests run in parallel more often. It may also improve the revocation situation depending on which direction #179's solution takes.
If we decide to support hot reloading to be more consistent with how certificates are verified on macOS and Windows (which hold no in-process trust anchor state between requests), we can utilize Kotlin's more granular RwLock support to optimize for the concurrent reader case while still allowing state updates to be processed safely.
Relates to #66