Bad synchronization?

[hmijail]

https://github.com/ConsenSys/cava/blob/512b0b66fe23ac4dd192f0fff5e211f79a5c41a5/net/src/main/java/net/consensys/cava/net/tls/FingerprintRepository.java#L56

addFingerprint() writes to fingerprints with synchronization, but reads it without synchronization through contains(String, Bytes). This seems to be a bug. If asymmetric read/writer locking is wanted, then probably a ReadWriterLock should be used.

[cleishm]

Hi @hmijail!

This should be ok as we're avoiding any locks on reading, and the map is never updated - it is only ever replaced (replacing the object reference). Admittedly, it is not currently marked as a volatile reference, so there will be no guarantees that the results of addFingerprint() would be immediately visible to other threads, but this is unlikely to be a concern in practice. Still, to alleviate that and also add extra documentation, I've created #40.

Thanks for pointing this out!

[hmijail]

(Deleted and reposting to fix / clarify)

Hi again,

I believe that the new version will behave correctly, but that it might be needlessly complicated. I'm trying to understand the use case. What is the goal?

1. to avoid updating a live HashMap while there might be active readers, with only 1 updater thread
2. to avoid having 2 threads updating fingerprints in parallel

For (1), I believe that building the new HashMap aside and publishing it with the volatile fingerprints variable is enough for correctness. The synchronized block is superfluous.

The synchronized block is only helpful for (2).

Apart from that, #40 also adds an unmodifiableMap. It's an interesting idea, but then looks like fingerprints is first published as a HashMap, but after addFingerprint() it will suddenly turn into an unmodifiableMap. Maybe it'd be more coherent to make fingerprints an unmodifiableMap from the beginning?

[cleishm]

Hi @hmijail!

Thanks for your interest!

The synchronized block exists to avoid concurrent attempts to write the backing file that contains the host list. If two threads try to add a host concurrently, then they will be serialized. Without that, then they'd both write and one update would vanish when the other does the atomic file replace.

You are correct about #40 and the unmodifiableMap inconsistency. The use of unmodifiableMap was added in #40 just for safety and to make it obvious that the map is never modified after creation. To be consistent there should also be an unmodifiableMap wrapper in the result of parsing the file in the first place, just as added protection. I've added that in #41.

And no, unmodifiableMap doesn't have any magic internal knowledge. It just delegates only read methods to the underlying map and throws on any write methods. If you directly modify the underlying map, then it will appear changed even via the unmodifiable proxy.

Thanks again!

[hmijail]

Oh, sorry, I modified my comment not realizing that you had answered. Thank you for the explanation :)