Skip to content

[Thread Safety] - The setLogging(to:) method is not thread safe

From Cory on the SwiftNIO team in Vapor's Discord: (thread)

Here are two parts of the protocol requirements for RedisClient:

Now, RedisClient doesn't actually say whether conforming types have to be thread-safe, but given that RedisConnection is clearly trying to be, I've inferred that it is The problem here is, assuming that RedisClient is supposed to be thread-safe, there are two awkward requirements placed on this code:

  1. It must be possible for a conforming RedisClient to synchronously hand you its logger, on any thread.
  2. It must be possible for you to set the RedisClient's logger from any thread, though there's no requirement that this be synchronous. Because of the synchronicity requirements of (1), in practice all conforming RedisClient implementations must wrap their logger in a Lock RedisConnection does not It sets the logger without synchronization: https://gitlab.com/Mordil/RediStack/-/blob/master/Sources/RediStack/RedisConnection.swift#L310-312 And reads it without synchronisation: https://gitlab.com/Mordil/RediStack/-/blob/master/Sources/RediStack/RedisConnection.swift#L102-106

I think the reason this hasn't come up thus far is that, in practice, no-one is actually setting the logger Now, there are two options

  1. Make all accesses to RedisConnection.logger go through a lock. This is pretty sad! In general the cost isn't that bad because most of the time people don't actually read or write the logger, but still.
  2. Just remove the ability to read/write the logger. I don't really know why the protocol allows you to do this. Is there a compelling reason?

(Side note: 2 is only an option because RediStack hasn't tagged 1.0.0 yet)

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information