search

DUNE-DAQ / iomanager

Package providing a unified API
0 stars 0 forks source link

Resource access locking #3

Closed roland-sipos closed 1 year ago

roland-sipos commented 2 years ago

@eflumerf many thanks for the fixes for thread safe access. These changes raised a question... do we really want to go down the way of access based scope locking: https://github.com/DUNE-DAQ/iomanager/commit/598332bbd8f141700e7643e1c919b00879e2569c

Or having concurrent containers for the underlying objects in the QueueRegistry and NetworkManager naturally solve the issues with concurrent access?

I think excessive locking for receiving/sending may result with a subtle performance impact. I still like the way for scope locking adding callbacks and creating resources, but wouldn't it make more sense to have these "creation"/instantiation thread safety mechanisms in the corresponding registries (QueueReg/NetworkMan)?

Food for thought. I'm adding few people on the watchlist.

eflumerf commented 2 years ago

I'm certainly open to using an object-scope mutex instead of a static mutex in the Sender, though I'll note that since it is in a template function, the static scope is only for Senders of that message type.

eflumerf commented 2 years ago

Phil noticed over-locking in the Trigger app context, so I'm making the send/receive mutexes member variables within Network{Sender,Receiver}Model. (PR #7)

dingp commented 2 years ago

@roland-sipos @bieryAtFnal Which release should this issue be targeted to?

bieryAtFnal commented 2 years ago

@dingp , it would be great for @roland-sipos and @eflumerf to comment on this Issue... My sense is that subsequent changes, which were motivated by observed behaviors in the system, helped define/refine the behavior that we want, for example iomanager PR #7, and this Issue can be closed as a result of those additional changes that happened after this Issue was filed.

eflumerf commented 1 year ago

Closing based on Kurt's comments and length of time elapsed.