MDSplus / mdsplus

The MDSplus data management system
https://mdsplus.org/
Other
72 stars 44 forks source link

MdsIP locking #1124

Closed tfredian closed 6 years ago

tfredian commented 6 years ago

It seems that the whole of the mdsip code could be greatly enhanced by adding connection locking capability. There are various potential race conditions that could be solved with proper locking. Some of the problems are obvious and there may be high level locks in applications to avoid these but there should be better locking down in the lower levels of the mdsip code. For instance when doing MdsValue to evaluate an expression on a remote server, the transaction usually requires sending a few messages to the server and then retrieving an answer. If an mdsip connection is shared by multiple threads in an application, those threads should take out a lock on a connection for the duration of the MdsValue transaction to ensure that all the I/O for that traction is completed sequentially before another transaction can commence.

There is also a potential issue if for some reason one thread tries to close an mdsip connection while another thread has a transaction in progress on the same connection. Since the closing of a connection frees some data structures related to the connection it is possible that another thread might erroneously try to access this memory if the connection was closed by another thread.

I believe adding a much better locking scheme would be worthwhile but it would likely be fairly large effort. If it was done well then we should investigate whether some existing locking mechanism in the code should be altered or removed.

drjdpowell commented 6 years ago

I thought I would comment as I've recently written a LabVIEW wrapper API for MdsIP (unrelated to the other LabVIEW implementations). I had to introduce a LabVIEW-level lock about all actions, so that only one public function could be in operation at any one time. This was true even for unshared independent connections, as the library stores the connection ID in a global variable (if the connection ID could instead be passed into functions as a parameter, I might be able to do "transactions" in parallel on separate connections).

GabrieleManduchi commented 6 years ago

If you use the mdsobject C++ library you can use mdsip in multithreaded applications since the connection id is not static, but embedded in the Connection object instances. The MDSplus LabVIEW interface uses Connection objects wrapped into Connection VIs and there is no need of LabVIEW locks to handle even multiple concurrent mdsip connections.

Look at http://www.mdsplus.org/index.php?title=Documentation:Tutorial:RemoteAccess&open=40368015397376491519&page=Documentation%2FThe+MDSplus+tutorial%2FRemote+data+access+in+MDSplus for an introduction to Connection MDSplus objects.

                             Gabriele

-- Gabriele Manduchi

Istituto Gas Ionizzati del CNR Consorzio RFX - Associazione EURATOM/ENEA sulla Fusione Corso Stati Uniti 4, 35127 Padova - Italy ph +39-049-829-5039/-5000 fax +39-049-8700718 mailto:gabriele.manduchi@igi.cnr.it, http://www.igi.cnr.it

On 29/11/2017 00:07, drjdpowell wrote:

I thought I would comment as I've recently written a LabVIEW wrapper API for MdsIP (unrelated to the other LabVIEW implementations). I had to introduce a LabVIEW-level lock about all actions, so that only one public function could be in operation at any one time. This was true even for unshared independent connections, as the library stores the connection ID in a global variable (if the connection ID could instead be passed into functions as a parameter, I might be able to do "transactions" in parallel on separate connections).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MDSplus/mdsplus/issues/1124#issuecomment-347694698, or mute the thread https://github.com/notifications/unsubscribe-auth/AISySHb1ekn_k7irK3JOi7DuS5mXM-rnks5s7JIVgaJpZM4QcWWZ.

drjdpowell commented 6 years ago

We had looked at that library, but decided against using it, and instead made a much simpler to use API that uses Variants. It wraps MDSLib.dll.

drjdpowell commented 6 years ago

An example function, showing the API differences: put

tfredian commented 6 years ago

The mdsip locking issues should now be solved by two pull requests: https://github.com/MDSplus/mdsplus/pull/1163 https://github.com/MDSplus/mdsplus/pull/1168

The second of these added new entry points to MdsLib which have the names xyzR for each of the standard MdsLib API's. These new entry points take a connectionid as the first argument and these do not use or set the Global connectionid therefore making them much more thread safe than the original API. There is also now locking on mdsip transactions on each connection so that if one thread begins a transaction such as those done my MdsValue, the complete transaction of sending expression info to the server followed by retrieving of the answer will be locked to prevent another thread from interfering with that transaction should it be communicating with the server over the same connection. These changes should now be in the alpha releases of MDSplus.