Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2: Should we handle errors from signals that don't handle CancelledError #1152

Open
callumforrester opened this issue Aug 21, 2023 · 3 comments
Labels

Comments

@callumforrester
Copy link
Contributor

Currently in v2 signals are expected to handle CancelledError on timeout and raise NotConnected

async def connect(self, ...):
    try:
        _connect(...)
    except CancelledError:
        raise NotConnected("reasons")

If we do not do this, then a timeout results in the whole program crashing with CancelledError. It is an easy mistake to make and a hard one to solve if the user has not read all of the documentation. It might be worth trying to come up with a more robust design.

In GDA, authors of new Scannables are expected to handle InterruptedException themselves. If they do not, scans do not behave as expected and can become deadlocked and unkillable. Many Scannables were written without handling the exception leading to strange and hard-to-diagnose behaviour. We should nip the same problem in the bud here.

Paging @coretl @evalott100 @RAYemelyanova and @danielballan

@rosesyrett
Copy link
Contributor

rosesyrett commented Aug 22, 2023

Could I ask where this code snippet is from? I thought CancelledError is actually an asyncio.CancelledError, i.e. the _connect there should be able to raise it's own custom errors.

@callumforrester
Copy link
Contributor Author

It's not directly from the code, it is a highly simplified version of Signal.connect() where the signal is using an epics backend, with all the indirection etc. removed. It is more generally how we are expected to write .connect()

@coretl
Copy link
Collaborator

coretl commented Aug 29, 2023

The reason behind NotConnected is purely for squashability. Any NotConnected that bubble up to DeviceCollector are squashed into a summary, while others are reported with their tracebacks. If anything fails to connect then with DeviceCollector() raises

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants