Thread safety of the getters within Narayana configuration EnvironmentBeans

47 views
Skip to first unread message

Ondra Chaloupka

unread,
Jun 2, 2020, 10:46:01 AM6/2/20
to narayana-users
Hi,

recently we have've got a discussion about integration of Narayana with Agroal and we've got to a discussion about Narayana thread safety of the getters in the Narayana EnvironmentalBeans. I would like to check here if it's something to concern about or not at all.

Basically, the environment beans (e.g. JTAEnvironmentBean - https://github.com/jbosstm/narayana/blob/5.10.5.Final/ArjunaJTA/jta/classes/com/arjuna/ats/jta/common/JTAEnvironmentBean.java#L51) use a collections to store configuration properties. For example list of XAResourceRecoveries. The setter and getter are synchronized and the setter returns the copy of the Collection  but the objects themselves (returned inside of the Colletion) could be mutable. Beside that there are some getters which return directly the created collection without the use of the copy constructor.

I limited my initial investigation only for JTAEnvironmentBean and I can see the returning of the collection of the mutable objects consist either recovery functionality or cmr functionality.
The recovery processing takes the lists everytime when recovery cycle starts. Despite the objects are mutable it seems quite safe to me.
For CMR functionality I'm not so sure.

I would like to check with you if this was somehow considered and/or if it's something that could be potential issue (if so how to approach it)?

Thanks
Ondra

Tom Jenkinson

unread,
Jun 2, 2020, 12:08:58 PM6/2/20
to narayana-users
Which element of CMR concerns you here? I think we would need to inspect the code in greater depth to understand if there is a problem with those getters that do not wrap in a new collection need to do that.

Ondra Chaloupka

unread,
Jun 4, 2020, 4:25:32 AM6/4/20
to narayana-users
Regarding the CMR,  I was pointing to all the JTAEnvironmentBean CMR getters which provides the direct instance of the configured object, ie.

I think those ones should be considered for using the copy-constructor of the returned collections.

Regarding the first part of my question - returning a copy of map of mutable objects - I assume this was designed with consideration of implications of such state.

Ondra

Tom Jenkinson

unread,
Jun 4, 2020, 5:19:53 AM6/4/20
to Ondra Chaloupka, narayana-users
On Thu, 4 Jun 2020 at 09:25, Ondra Chaloupka <ocha...@redhat.com> wrote:

I think that would be useful to make them consistent. It doesn't look like they expect to be changed after first setting at the moment.

Regarding the first part of my question - returning a copy of map of mutable objects - I assume this was designed with consideration of implications of such state.

I think the approach may have possibly started at https://issues.redhat.com/browse/JBTM-596, perhaps Jonathan will recall the motivation?


Ondra


On Tuesday, June 2, 2020 at 6:08:58 PM UTC+2, Tom Jenkinson wrote:
Which element of CMR concerns you here? I think we would need to inspect the code in greater depth to understand if there is a problem with those getters that do not wrap in a new collection need to do that.

On Tuesday, June 2, 2020 at 3:46:01 PM UTC+1, Ondra Chaloupka wrote:
Hi,

recently we have've got a discussion about integration of Narayana with Agroal and we've got to a discussion about Narayana thread safety of the getters in the Narayana EnvironmentalBeans. I would like to check here if it's something to concern about or not at all.

Basically, the environment beans (e.g. JTAEnvironmentBean - https://github.com/jbosstm/narayana/blob/5.10.5.Final/ArjunaJTA/jta/classes/com/arjuna/ats/jta/common/JTAEnvironmentBean.java#L51) use a collections to store configuration properties. For example list of XAResourceRecoveries. The setter and getter are synchronized and the setter returns the copy of the Collection  but the objects themselves (returned inside of the Colletion) could be mutable. Beside that there are some getters which return directly the created collection without the use of the copy constructor.

I limited my initial investigation only for JTAEnvironmentBean and I can see the returning of the collection of the mutable objects consist either recovery functionality or cmr functionality.
The recovery processing takes the lists everytime when recovery cycle starts. Despite the objects are mutable it seems quite safe to me.
For CMR functionality I'm not so sure.

I would like to check with you if this was somehow considered and/or if it's something that could be potential issue (if so how to approach it)?

Thanks
Ondra

--
You received this message because you are subscribed to the Google Groups "narayana-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to narayana-user...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/narayana-users/038212b6-6dd7-4f0f-900c-54390c85528f%40googlegroups.com.

Ondra Chaloupka

unread,
Jun 26, 2020, 9:08:12 AM6/26/20
to narayana-users
I created an issue regarding the CMR copy constructors: https://issues.redhat.com/browse/JBTM-3338

Tom Jenkinson

unread,
Jun 26, 2020, 10:07:20 AM6/26/20
to Ondra Chaloupka, narayana-users
Thanks for following up Ondra

Michael Musgrove

unread,
Jun 29, 2020, 5:33:24 AM6/29/20
to narayana-users
Users of CMR are normally application containers so just adding a note to the javadoc should be enough.
You asked a question of Jonathan early on in the thread, did you follow up on that?

Tom Jenkinson

unread,
Jun 29, 2020, 6:35:23 AM6/29/20
to Michael Musgrove, narayana-users
The initial mention of Jonathan came from me with regards to JBTM-596 possibly being the start of the approach but I have not followed up on that.

Ondra Chaloupka

unread,
Jun 29, 2020, 9:16:52 AM6/29/20
to narayana-users
I hope the change, provided as the fix for JBTM-3338, is worthy and it fixes the possible thread safety problem and alignes the JTA bean with the code practice which that bean was started with.
I'm asking Mike and Tom for the review in the associated PR.
Reply all
Reply to author
Forward
0 new messages