envoyproxy / java-control-plane

Java implementation of an Envoy gRPC control plane
Apache License 2.0
293 stars 136 forks source link

cache: Make SimpleCache#watches thread safe #31

Closed snowp closed 6 years ago

snowp commented 6 years ago

SimpleCache#watches publishes a non-thread safe member of SimpleCache without synchronization which means that changes made on other threads are not guaranteed to be visible.

This change synchronizes access to the watches method and makes a defensive copy of the underlying data. To simplify this I changed the data structure from Map<Map<>> to Table, which let's me use ImmutableTable.copyOf to copy the data more easily.

It also changes usages of #watches() internal to the class to reference the field directly to avoid the cost of a copy.

Signed-off-by: Snow Pettersen snowp@squareup.com

This method is just used without synchronization in test right now, so it's probably not a big deal, but I figured it'd make sense to keep the class completely thread safe, even if just to prevent bugs in the future when someone makes this method public and to ensure that we don't get any concurrency issues in test.

cc @nicktrav

codecov-io commented 6 years ago

Codecov Report

Merging #31 into master will increase coverage by 0.63%. The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #31      +/-   ##
===========================================
+ Coverage     94.97%   95.6%   +0.63%     
+ Complexity       44      43       -1     
===========================================
  Files             7       7              
  Lines           179     182       +3     
  Branches         15      13       -2     
===========================================
+ Hits            170     174       +4     
  Misses            4       4              
+ Partials          5       4       -1
Impacted Files Coverage Δ Complexity Δ
.../io/envoyproxy/controlplane/cache/SimpleCache.java 96.82% <100%> (+1.82%) 16 <3> (-1) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 164eb29...e5503bb. Read the comment docs.

joeyb commented 6 years ago

@snowp - LGTM, just needs a rebase against master.

snowp commented 6 years ago

rebased