Code review horror in 4 lines of code
I run into the following code during code review and had an immediate and visceral reaction.
This is a (bad) attempt to add thread safety, because you are getting a value through a read only interface, but there is still the mutable instance to work with at the source, and now you have someone that observes the instance while it is being mutated, outside the lock.
The proper way to handle this is to copy the list (under the lock) and return a distinct copy.
Comments
True, but the
IReadOnlyList<T>
interface doesn't guarantee any immutability. It just prevents callers from modifying the contents of the list. If I need to guarantee immutability, I tend toIImmutableList<T>
. As a consumer, you cannot know that the content of theIReadOnlyList<T>
isn't modified concurrently ie. while you're using the list's enumerator.Would you use a method name like
GetReadOnlyCopyOfMessageList()
instead of a property, because that could potentially result in a lot of allocations for a simple property getter?Hangy,
In practice, this is a scenario where we are needing a snapshot of a mutable value, because we want to display the current state.Using ImmutableLIst is possible, but likely too expensive. Most of the time, you aren't going to be observing the state, doesn't make sense to pay the immutable cost. And yes, in this case a
GetMessagesSnapshot()
would probably be better.Hangy,
In practice, this is a scenario where we are needing a snapshot of a mutable value, because we want to display the current state.Using ImmutableLIst is possible, but likely too expensive. Most of the time, you aren't going to be observing the state, doesn't make sense to pay the immutable cost. And yes, in this case a
GetMessagesSnapshot()
would probably be better.You didn't even mention the use of
lock(this)
, which would be enough on its own to have a pull request rejected here unless there was a very good (and documented) reason to do it.Two things that came to my mind while reading the code:
1. The caller could cast the
IReadOnlyList<T>
to aList<T>
to get full access to the internal list - one more reason to make a copy of it and return the copy;2.
lock(this)
is considered a bad practice. See the Remarks section in https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2012/c5kehkcz(v=vs.110)None & Jorge,
The avoid
lock(this)
issue is a recommendation when you are developing things that are used in libraries. This is running in the RavenDB process, so fully controlled by us. The cost of additional allocation for a lock object is too costly to pay.Comment preview