Discussing the design, decisions and discovery involved in developing Mutability Detector, an open-source analysis tool for Java.

Sunday 15 July 2012

Checking for Mutability in JSR 310: ZoneOffsetTransitionRule


Another example of a failing test case from ThreeTen. In this case, the assertion:

assertImmutable(javax.time.zone.ZoneOffsetTransitionRule.class);

Fails with the message:


org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: javax.time.zone.ZoneOffsetTransitionRule to be IMMUTABLE
     but: javax.time.zone.ZoneOffsetTransitionRule is actually NOT_IMMUTABLE
    Reasons:
        Field is not final, if shared across threads the Java Memory Model will not guarantee it is initialised before it is read. [Field: timeEndOfDay, Class: javax.time.zone.ZoneOffsetTransitionRule]
        Field can have a mutable type (javax.time.ZoneOffset) assigned to it. [Field: standardOffset, Class: javax.time.zone.ZoneOffsetTransitionRule]
        Field can have a mutable type (javax.time.ZoneOffset) assigned to it. [Field: offsetBefore, Class: javax.time.zone.ZoneOffsetTransitionRule]
        Field can have a mutable type (javax.time.ZoneOffset) assigned to it. [Field: offsetAfter, Class: javax.time.zone.ZoneOffsetTransitionRule]
    Allowed reasons:
        None.


Once again, the String Problem is biting us. In this case, mutability is seen to flow back from the type javax.time.ZoneOffset. There is a separate test case for ZoneOffset, which is only considered mutable because it has a String field. Thus, we can change the assertion to look like this:

assertInstancesOf(javax.time.zone.ZoneOffsetTransitionRule.class, 
                  areImmutable(),
                  provided(ZoneOffset.class).isAlsoImmutable());

Which leaves us with the single error, that the boolean field timeEndOfDay is not final. In this case, I think Mutability Detector has found a real problem. The field is never reassigned, so callers cannot change the object. However, instances of ZoneOffsetTransitionRule could be published to other threads, without extra synchronisation around the timeEndOfDay field, Without guarantees of visibility from the Java Memory Model, users could see it change from its default value (false) to its real value (true or false) once it's been made visible correctly.

VERDICT: True positive (yay)
WHAT TO DO: Change the field to be final. Making it volatile would also prevent the aforementioned problem, but Mutability Detector wouldn't take account of it (see the previous post). All the other fields in the class are final anyway, so it makes sense to have this one be final as well.

ThreeTen: 2, Mutability Detector 2

Next: ResourceZoneRulesVersion

1 comment:

  1. Fixed https://github.com/ThreeTen/threeten/commit/60ad0f24569491ab44a26e929c6e6c7e26b62244

    ReplyDelete