Conversation
Barecheck - Code coverage reportTotal: 96.86%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 |
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.
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 :)
Some newly-added tests in this branch needed to be updated after rebasing. This was because they passed a dictionary of Things to `ThingServer`'s constructor. The tests now correctly use `from_things` and are otherwise unchanged.
I've added text to the ActionDescriptor docstring directing the user to the global lock, in case they need to access it manually. This is in response to review comment from @julianstirling.
This splits out the context manager that holds the lock from the action function, when the action is called over HTTP. That means we will only log a single line if an action fails to start because of the global lock. I have deliberately left other global lock errors (e.g. for when an unlocked function calls locked code) to be handled as usual (i.e. log a traceback) because in my view that is an error, and should be handled by the action in the usual way. Re-raising the error as an InvocationError is appropriate, if terminating because the lock is busy is something we expect to happen.
The public API `hold_global_lock` no longer uses the True/False/None scheme, and instead has a more explicitly-named argument, as suggested by @julianstirling.
|
For now, if a Calling |
|
@julianstirling I've made all the suggested changes, so hopefully this is straightforward for you to rereview. @bprobert97 there is reasonable testing for this, I think, but I'm very open to suggestions if you think we can do better - hopefully in a make-an-issue-and-fix-it-at-our-leisure sort of way, but now feels like a sensible time to ask for another pair of eyes, either pre- or -post-merge. |
This fixes several things: * Adds a test for the expected error if an un-acquired lock is released. * Adds a test that the global lock is the same lock. * Adds a test for non-default timeouts. * Fixes some docstrings. * Clarifies a confusing comment. Thanks @bprobert97
bprobert97
left a comment
bprobert97
left a comment
There was a problem hiding this comment.
Happy with changes! Thanks Richard.
3 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