Overall I generally agree and as far as I have ever heard using the
concurrent form of hash-map rather than the non-concurrent one is a minimal
overhead if at all, so why not.
The way most of these Maps are used are already concurrency safe. So as
long as we mean to simply keep this in mind when creating new code and
changing up the existing code then +1
On Thu, Oct 29, 2020 at 10:57 AM Sanne Grinovero
wrote:
> 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
___
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
