Hi all,
I'm inspecting some old code, and regularly finding a pattern which
isn't entirely correct; it's not a big deal as it seems to not cause
any problems so far in practice (expect the one I'm working on!), but
I'll share this in hope we can slowly improve such things while
evolving the codebase.
When making an immutable, internal component we'll often have many
fields with a "final" qualifier. This is good and we should definitely
keep doing it.
But also, one needs to ensure that once the constructor of an object
has completed, such objects referenced in the final field should no
longer be mutated (unless of course they are mutated in a thread safe
way as well).
To get more concrete, the following pattern is frequent in our code,
but should not be used:
class A {
private final HashMap state = new HashMap();
A() { //constructor
}
public initState( p ) {
state.put (p, v);
}
}
There's many reasons for this; at this stage my concern is mostly that
the actual state which we're writing in the _state_ field doesn't
benefit from the effect of the final field on visibility, as by
specification the "freeze" of state is applied when the constructor
has concluded.
an additional problem is that such protected methods get to work on a
partially constructed object as the constructor hasn't finished being
invoked yet; this leads to additional subtle errors when the method is
overridden by subclasses or relying on state from superclasses.
A better version could be:
class A {
private final Map state;
A(params) { //constructor
this.state = initState( params );
}
static Map initState( params ) {
HashMap state = new HashMap();
for ( some_loop on params ) {
state.put (p, v);
}
return state;
}
}
If this doesn't work in your case as you need to "collect" some data
by passing A to various other services, like we frequently need, this
would suggest that you need an intermediary object, such as a builder
pattern: collect all things you need, then build the immutable final
representation of the state you need and make sure the builder is
discarded.
Alternatively, that "state" Map could use a ConcurrentHashMap if your
service actually needs runtime state mutation by design.
Please take this in consideration, as it will allow us to write better
maintainable code, unlocks some possible optimisations which are
otherwise hard to implement, and above all is actually correct in
regards to the Java memory model.
In the above example, it's interesting to highlight that the current
code is working fine as we eventually publish references to A over a
barrier, which will publish the state of the post-initialized A.state
, so there's no rush in fixing such things BUT when I need to make
changes to such patterns it's challenging to trace the barriers to
make sure I'm not inadvertently introducing an obscure issue. Would be
best to not rely on such root barriers.
For those who want to know more, a good reference is Chapter 17.5 of
the Java Language Specification.
Thanks!
Sanne
_______________________________________________
hibernate-dev mailing list -- hibernate-dev@lists.jboss.org
To unsubscribe send an email to hibernate-dev-le...@lists.jboss.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
