code-review-checklists/java-concurrency

Meaningless example in LI.3

leventov opened this issue · 0 comments

As malkusch noted here:

https://github.com/leventov/java-concurrency-checklist#safe-local-dcl

The above code could be fixed as follows: void cleanup() { MyClass lazilyInitialized = this.lazilyInitializedField; if (lazilyInitialized != null) { lazilyInitialized.close(); } }

That is surely a fix to the NPE, but is that really a fix? I think there is no guararantee that a thread might see that non volatile field to make that local assignment in the first place, thus the if block might never be entered by another thread.

I think this is a valid issue, the example doesn't make a lot of sense right now.

I'm not sure how to fix it, yet. Perhaps it should be not a cleanup() but some other kind of method. Or instead of pointing to potential NPE issue, the warning about non-volatile lazily initialized fields should point to visiblity issues instead. However, there is the same visibility issue if the field was volatile, too.