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

fix: Release lock before raising exception for improved recovery #5941

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fabiomadge
Copy link
Collaborator

@fabiomadge fabiomadge commented Nov 28, 2024

Fixes #5913.

@keyboardDrummer
Copy link
Member

Could you add a test?

@fabiomadge
Copy link
Collaborator Author

I think that's more trouble than it's worth. The map already has tests and this change is just about what I consider undefined behaviour.

@keyboardDrummer
Copy link
Member

keyboardDrummer commented Nov 29, 2024

this change is just about what I consider undefined behaviour.

If I understand correctly, previously you could call Put incorrectly, and then if you catched the error that it throws, any subsequent calls to Put would hang. Did I get that right? I agree it's a corner case, but one that a user ran into and that seems easy to test. Why not add a test to prevent it from re-occurring?

@fabiomadge
Copy link
Collaborator Author

Your understanding is correct. I'm not against adding the test, but I’m not convinced its worth my time.

@fabiomadge
Copy link
Collaborator Author

Let me elaborate on what I meant by undefined behaviour. I can't think of a way of running into this issue from Dafny generated code. I believe the contract with the user stipulates that we provide a correct Dafny library, not one for general consumption from any Python codebase.
I also can't think of a precedent of tests without any Dafny code, so I am hesitant to break with it.

@keyboardDrummer
Copy link
Member

Let me elaborate on what I meant by undefined behaviour. I can't think of a way of running into this issue from Dafny generated code. I believe the contract with the user stipulates that we provide a correct Dafny library, not one for general consumption from any Python codebase.

That's fair.

I also can't think of a precedent of tests without any Dafny code, so I am hesitant to break with it.

Me neither. You could use the littish tests for it but it would feel kind of hacky because you'd have a .dfy file with no actualy Dafny instructions, just shell commands to run some Python code.

@keyboardDrummer keyboardDrummer enabled auto-merge (squash) December 2, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python MutableMap should release locks if unhandled exception is raised
2 participants