access to dataset metadata not threadsafe

[scottslewis]

The implementations of getMetadata(Class) addMetadata(T)/setMetadata and any of the *Metdata methods on the LazyDatasetBase class do not have any synchronization around the access to the 'metadata' field...wrt creating the Map or accessing elements (getting and setting). This means that the IDataset instances are not thread safe, and multithreaded access could result in non-deterministic NPEs or ConcurrentModification exceptions.

It would be helpful for some use cases to make LazyDatasetBase.*Metdata thread safe.

[PeterC-DLS]

Thanks for the issue. Yes, it should be though we haven't required it so far. Do you have time to fix this?

[gerring]

If we do this we should avoid putting synchronised methods and objects everywhere because slowness of january is an issue already.

…

On 14 December 2016 at 01:24, Scott Lewis ***@***.***> wrote: The implementations of getMetadata(Class) addMetadata(T)/setMetadata and any of the *Metdata methods on the LazyDatasetBase class do not have any synchronization around the access to the 'metadata' field...wrt creating the Map or accessing elements (getting and setting). This means that the IDataset instances are not thread safe, and multithreaded access could result in non-deterministic NPEs or ConcurrentModification exceptions. It would be helpful for some use cases to make LazyDatasetBase.*Metdata thread safe. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#27>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQfbNSnXCooIVzClE97WmuzlFRft_lRks5rH0VigaJpZM4LMba8> .

[jayjaybillings]

We avoid synchronized methods as much as possible in other projects and use Atomics instead. Some studies have shown them to be eight times faster than synchronized methods because they are implemented in hardware. Jay Jay Billings Oak Ridge National Laboratory Twitter Handle: @jayjaybillings

________________________________ From: Matthew Gerring <notifications@github.com> Sent: Wednesday, December 14, 2016 4:11 AM To: eclipse/january Subject: Re: [eclipse/january] access to dataset metadata not threadsafe (#27) I we do this we should avoid putting synchronised methods and objects everywhere because slowness of january is an issue already.

On 14 December 2016 at 01:24, Scott Lewis ***@***.***> wrote: The implementations of getMetadata(Class) addMetadata(T)/setMetadata and any of the *Metdata methods on the LazyDatasetBase class do not have any synchronization around the access to the 'metadata' field...wrt creating the Map or accessing elements (getting and setting). This means that the IDataset instances are not thread safe, and multithreaded access could result in non-deterministic NPEs or ConcurrentModification exceptions. It would be helpful for some use cases to make LazyDatasetBase.*Metdata thread safe. - You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#27>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQfbNSnXCooIVzClE97WmuzlFRft_lRks5rH0VigaJpZM4LMba8> .

- You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#27 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGFVYX_MEdtvY57IsCH1hmboJn7b3Dsbks5rH7LYgaJpZM4LMba8>.

[gerring]

Are Atomics things like ReetrantLock?

[jonahgraham]

No, Atomics are their own thing, stuff in java.util.concurrent.atomic. Here is the article from Brian Geotz (author of Java Concurrency in Practice) from when the new feature was added in Java 5: http://www.ibm.com/developerworks/library/j-jtp11234/

[scottslewis]

Hi Matt. Using synchronized keyword is essentially a Reentrant lock (in python threading lib terms). You can also use the concurrent API (especially java 8), but my suggestion would be to use synchronized keyword in necessary places. I would offer to provide a patch, but just can't at the moment.

[scottslewis]

I hadn't noticed discussion above...wrt synchronized keyword. Although I agree that using synchronized in many locations is bad, I don't think that is necessary to have many uses of synchronized to satisfy...rather just protecting the add/removal/lookup to the metadata storage (preventing ConcurrentModificationExceptions) is what we are looking for.

[gerring]

Scott and Jonah, Thanks, duly noted, in a non-ironic way. Matt

…

On 2 February 2017 at 22:37, Scott Lewis ***@***.***> wrote: Hi Matt. Using synchronized keyword is essentially a Reentrant lock (in python threading lib terms). You can also use the concurrent API (especially java 8), but my suggestion would be to use synchronized keyword in necessary places. I would offer to provide a patch, but just can't at the moment. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQfbMiHkhPHzYc8y5LCNe9pxSSl8A8-ks5rYlqwgaJpZM4LMba8> .

[jonahgraham]

@PeterC-DLS PR #71 seems to have fixed this issue. Is there anything else left in this bug report that needs resolving?

[scottslewis]

My answer to Jonah is no...the fix in 71 looks good to me. Thanks for the work.

[PeterC-DLS]

Done with merged PR