Conversation
Barecheck - Code coverage reportTotal: 96.76%Your code coverage diff: 0.12% ▴ Uncovered files and lines |
|
I've now got passing tests with a Either way, I need to figure out how to handle the |
Before I tidied up error handling for properties, the error handling behaved differently between the testclient and the HTTP server (the testclient-based test was raising As has been mentioned elsewhere, it would be nice to add an integration test suite in addition to the OFM suite - largely because we can include more edge cases designed to test more rigorously. I am not proposing to do that here, as I think the |
This commit adds: * A server config key for the global lock * A class to implement the global lock * Context managers used when running actions or setting properties, to acquire said global lock Properties will either use the lock for setting, or not use the lock. If functional properties should use the lock for reading, they can acquire it in the getter. No documentation yet, but there are some tests for the lock. We still need some tests of the lock in the context of properties and actions, including the argument to opt out.
I've added tests for the ThingServerInterface functions related to the lock. This revealed a bug that is now fixed. I also added a convenience function to mock Thing instances, which provides a few attributes that are needed by the various descriptors and should help keep test suites tidy.
This now checks both directly and in the context of a `ThingServer` (simulated by a `TestClient`) that the global lock works as expected, if it is enabled.
I've now installed a FastAPI error handler that will provide a more graceful error when the global lock is not available. Errors are handled for actions by the normal mechanism, but for property code any exceptions previously resulted in a 500 Internal Server Error with no details. I now explicitly catch `GlobalLockBusyError` and provide a 409 response with a message.
I thought it was elegant to call the lock context manager and bind just the generator object it returns to the wrapped function. Unfortunately, said generator object may not be re-used. I have now added a test (that started off failing) to reproduce that problem, and have changed the offending code to use a fresh generator each time. This was caught by the OFM test suite :)
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
Looks nice. I think the point about hold_global_lock(False) we should discuss in the call.
| if global locking is enabled. That is appropriate if the action does | ||
| not have side effects that would cause problems for other actions, or | ||
| if more nuanced locking behaviour is required meaning the lock is | ||
| acquired directly in the action code. |
There was a problem hiding this comment.
May be good to point to which function is used to acquire it directly?
There was a problem hiding this comment.
refer to ThingServerInterface.hold_global_lock
There was a problem hiding this comment.
I've linked to hold_global_lock and also the section on the global lock (in preference to repeating that explanation here).
| """ | ||
|
|
||
|
|
||
| class GlobalLockBusyError(TimeoutError): |
There was a problem hiding this comment.
Does this log as error or exception? Seems like the sort of thing where we wouldn't want the traceback as it is an "expected" error during use (or reasonably foreseeable misuse!).
There was a problem hiding this comment.
Good question. It doesn't subclass invocation error so currently it will dump a stack trace. You're right it would be better to handle this differently, at least if it occurs in the action wrapper (i.e. if an action can't start because the server is busy).
If the exception happens in user code I think a stack trace probably is helpful, and if not the user should probably handle it.
| return self._get_server().global_lock | ||
|
|
||
| @contextmanager | ||
| def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: |
There was a problem hiding this comment.
This confuses me, I am not really sure the context where I would want to run with anything except True?
I can see that within internal LabThings code it might be useful i the enabled is set at the variable level. I think the exposed close to the user base shouldn't have an enable as the line:
with tsi.hold_global_lock(False):does not look like it would not try to get the lock.
I'd suggest the public function may to better to have the signature:
def hold_global_lock(self, *, error_if_unavailable:bool=True) -> Iterator[None]:This would hold the None and then True case of the current implementation.
2 participants
This PR adds:
Properties will either use the lock for setting, or not use the lock. If functional properties should use the lock for reading, they can acquire it in the getter. This is the default behaviour I discussed briefly with @julianstirling based on the idea that reading properties shouldn't change state. If getting a property needs to access hardware, this should be protected by a lock specific to the hardware - the stage and camera already have this in OFM.
It's possible that something similar to the
GlobalLockclass would be useful toThingsubclasses for other locks, because it supports use as a context manager with a default timeout. VanillaRLockinstances will block forever, which can cause undesirable queuing.We still need
ThingServerand using aTestClientso it's running in a real event loop.Closes #101
OFM-Feature-Branch: v3-labthings-global-lock