Missing @GuardedBy annotations in MapMakerInternalMap.java and RateLimiter.java

[GoogleCodeExporter]

We believe we have discovered some missing @GuardedBy annotations in 
MapMakerInternalMap.java and RateLimiter.java. Attached are an example of a 
missing @GuardedBy on a method and a missing @GuardedBy on fields.

The first diff corrects the missing @GuardedBy on a method.  
MapMakerInternalMap.Segment.removeCollectedEntry is not annotated as
@GuardedBy("this") though it accesses Segment.evictionQueue and 
Segment.expirationQueue which are both @GuardedBy("this").

The second diff corrects the missing @GuardedBy on fields. In RateLimiter.java, 
we noticed that the mutex lock is always held when these fields are accessed:

• RateLimiter.storedPermits
• RateLimiter.maxPermits
• WarmingUp.slope
• WarmingUp.halfPermits
• Bursty.maxBurstSeconds

and when the field RateLimiter.stableIntervalMicros is written to.  
Therefore, we speculate that RateLimiter.java is missing documentation about 
which fields are protected by a mutex.

We are interested in knowing if these annotations are an accurate description 
of the design. If so, we think they are desirable to better document the 
design. If these are useful, we can provide additional examples of missing 
annotations.

Original issue reported on code.google.com by jtha...@cs.washington.edu on 2 Apr 2014 at 6:49

Attachments:

• MapMakerInternalMap.diff
• RateLimiter.diff

[GoogleCodeExporter]

We don't consistently apply these annotations (it's really the original 
author's discretion)...

Dimitri: are you interested in taking a look at this for RateLimiter? (and 
Louis for MapMaker?)

Original comment by kak@google.com on 2 Apr 2014 at 6:52

[GoogleCodeExporter]

For context, my team wants to turn on static checking for @GuardedBy this 
quarter.  The reporter, Javier, works on the tool (the Checker Framework) that 
we will be using for this.  I believe he is starting development on the thread 
safety checker and is testing against Guava.

It would be great if Guava could be a test project as the Checker Framework 
team works on the thread safety checker.

Original comment by eaf...@google.com on 2 Apr 2014 at 9:01

[GoogleCodeExporter]

Ping.  Any thoughts on this from the Guava team?

Original comment by eaf...@google.com on 4 Jun 2014 at 10:27

[GoogleCodeExporter]

Original comment by kak@google.com on 5 Jun 2014 at 7:22

• Changed state: Accepted

[GoogleCodeExporter]

Recent restructurings to RateLimiter have made this trickier. The class is now 
split across two files (including SmoothRateLimiter), and the mutex is now 
lazily initialized (don't ask...) and accessed through a method. I would be a 
big fan of adding the annotations and an even bigger fan of seeing them 
enforced. I'd be happy to review a revised CL.

I believe that all the fields you've listed (and probably all the methods in 
your CL, though I didn't look as closely), including stableIntervalMicros for 
both reads *and* writes nowadays, deserve the @GuardedBy annotation.

Original comment by cpov...@google.com on 5 Jun 2014 at 8:52

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:09

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:17

• Changed state: Migrated

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07

• Added labels: Restrict-AddIssueComment-Commit