irmen / Pyro5

Pyro 5 - Python remote objects
https://pyro5.readthedocs.io
MIT License
305 stars 36 forks source link

Tagging methods as idempotent vs non-idempotent and edit locks #39

Closed michael-a-schmidt closed 3 years ago

michael-a-schmidt commented 3 years ago

Is it possible to implement something like an edit lock (for lack of a better term) so that only one client can invoke non-idempotent methods on a remote object? What I have in mind is something like using API keys for authorization for methods that can change the state of the system, while freely allowing other clients to use idempotent methods. This would be kind of like keeping routes that implement GET requests open to everyone but decorating routes that implement POST/PUT/DELETE requests with the '@token_auth.login_required' decorator in Flask.

My use case is that I would like to use Pyro5 to manage laboratory hardware. I have a python API for the different pieces of hardware, and an instrument manager class to wrangle them all. I'm planning to use Pyro5 and a name-server to make it easy for multiple processes (e.g. a GUI, measurement software, etc.) to access the instruments. The server will use instance_mode='single' so that there's only ever one instance of the instrument manager and instrument objects, and I'll use SERVERTYPE = 'multiplex' to serialize the incoming requests. I believe we can even use batch mode to execute blocks of calls to the instruments together. All processes will only ever be running on the local machine, so I don't have security concerns really.

My only concern now is that I want to make sure that only one process at a time can edit the state of the instruments. It would be really bad if during my measurement routine someone walked up to the computer and edited, for example, the laser output power using the GUI. I want the GUI to be polling the instruments so that I get visual feedback of what's going on, but I don't want to make any conflicts with the measurement recipe. I control all the software here, so I could in principle grey-out the buttons on the GUI when we're in measurement, but it seems like it would be a nice feature to have available to make it so that if a process that doesn't have the right to edit the state of the instruments tries to do so that you'd get an exception of some kind.

Thanks - and thanks for making such a great tool!

irmen commented 3 years ago

Not entirely sure if you expect Pyro itself to provide such a mechanism?

If not, (which I think is the way to go): this sounds like you're describing a distributed critical section locking mechanism. This can be a very hard problem to solve reliably, I think many CS papers have been written on the subject. However for your situation it sounds to me that you could build something that depends on some sort of "key" that is handed out by the "locking server" and that you can use once to access a resource for modification. Without a valid key, modification access is blocked. Another way is to devise some sort of group concensus algorithm that can determine who of the clients that request access is actually allowed, but I feel this doesn't really map onto your scenario.

By the way 1: distribued locking is harder in a truly distribued environment where calls and connections are unreliable. If you're running it all on a single machine, you can very likely cut corners and simplify enormously.

By the way 2: if you're already using multiplex servertype there's only one pyro server process at a time running (unless ofcourse you're manually spawning more daemons on other ports). If you can make sure there is at most ONE pyro daemon at all times, in multiplex mode, that is doing the modification requests, you are already done aren't you? All requests will be processed 1 at a time in serialized fashion

michael-a-schmidt commented 3 years ago

Thanks very much for the feedback, and I definitely did not expect this to be an out-of-the-box feature that Pyro would provide, this would definitely be something extra that I'll need to create. Because of that, I wasn't sure if creating an issue on Github was the right way to go, but I didn't see a forum or google group. Hopefully this isn't a problem - to clarify this definitely is not a bug or even feature request.

I definitely agree with your suggestion to use a "locking server" to hand out keys. I think this could actually just be implemented as a very simple flask app (with which I'm more familiar) that runs on the same, single machine and would expose an extremely simple API for generating and validating keys.

On the client side, I'll definitely have to implement some logic to get keys. This should be fairly straightforward to implement.

On the server side, yes there will only be one pyro server process, and one pyro daemon, so there's no possibility of two clients trying to set conflicting instruments settings simultaneously. All the requests are serialized, which is exactly the behavior I need.

I think there are basically two ways I could implement the logic to check for keys - on the client-side with some sort of wrapper around the existing pyro proxy, or on the server side by implementing a custom event loop. I think the client-side sounds easier to implement, especially if I use a proxy-to-a-proxy approach as suggested here:

https://stackoverflow.com/questions/33388868/make-pyro4-proxy-indexable

This wouldn't be the most elegant solution though, as I'd have to make sure to wrap the proxies myself.

Doing things on the server-side would make it more seamless I think, but I'm pretty sure that at a minimum I'd have to implement a custom event loop to do that so that I can check for a valid key on the server before executing the request. I'd also have to add logic to all the proxy objects to send their key along with the request to the server - perhaps this is more than what is needed in my case.

If you have any thoughts or suggestions on how to proceed I would very much appreciate it, and thanks again!

irmen commented 3 years ago

Hm wait-- if all the requests are already serialized and there's only once Pyro daemon, why do we still need the locks?

michael-a-schmidt commented 3 years ago

The issue is that our measurement routines might take several minutes to run to completion, and consist of many different calls to the instruments. For example, we might set a voltage during step 1, then perform 10 more steps on other instruments under the assumption that the voltage is known from step 1. If we group all of the measurement routine into a single batched call, then you're right, there's no problem - if for example someone accidentally changes the voltage setting through the GUI while the measurement is already running, the server would serialize the request and the measurement would be fully completed before the set voltage command is executed.

The only problem with this approach is that our GUIs would also completely stop updating while the measurement is running. Ideally I'd like the GUI to poll the instruments to get values every ~second or so even while the measurement is running, and for that to happen I would need to make sure that the full measurement is not executed as a single batch. The server would just serialize the requests and the measurement routine would be interleaved with the GUI 'getter' calls.

A simple step I can take is to break up the measurement routine into smaller groups of self-contained steps, and execute each group as a batch. This way the GUIs could continue to update between the batch calls, and that might be good enough - no custom code on the server-side would be needed at all for that.

irmen commented 3 years ago

So am I correct with my understanding that:

1) client A modifies some parameter 2) from now on all reads are okay, but no other modifications 3) only when client A releases its exclusive acces, other modifications can take place

?

michael-a-schmidt commented 3 years ago

Yes, that's exactly right - the desired behavior is that only one client (A) at a time can modify (and read), while all other clients can read only while client A has the lock.

irmen commented 3 years ago

That sounds exactly like a file system works with shared reads and exclusive writes. Maybe you can look into that how that works?

I don't think you'll need a third entity to serve the lock keys. Why not just add a method to your existing Pyro service to obtain an exclusive write / modify lock and also add a method to release it once it is safe to release. (The pyro object handling this must be a single instance mode for all calls otherwise you can't store persistent locking status across multiple calls)

michael-a-schmidt commented 3 years ago

Ah, yes, you're absolutely right - I had a chance to read up on this, and this approach will work perfectly for my situation. The one wrinkle is that I have to know which particular client has the lock, but I can identify the clients by doing:

from Pyro5.callcontext import current_context

do stuff, and in my instrument manager identify clients via:

this_client = current_context.client_sock_addr

Once I have that info I can check for the presence of a lock-file, and if so is the lockfile associated with the current client. I also won't have to worry about any race conditions in this situation because I'm using single-instance mode with a multiplexed server, so there's no chance another client would create a lock while I'm checking for the presence of the lock.

Then to make it neat and tidy, I can use a class decorator and method decorators to tag which of the methods in my instruments require the lock, and wrap those with a function that handles checking for the lock and raises an exception if a different client has it.

Thanks for your help on this, much appreciated, and thanks again for making a great project

irmen commented 3 years ago

Thank you. Sounds good, with the decorators!

You have an interesting application for Pyro and I'm glad the project is useful for you.

Closing this for now - feel free to reopen or comment again if necessary