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

More explicit error message if transport is already closed #35559

Merged
merged 11 commits into from
Jun 4, 2024
Merged

Conversation

lmazuel
Copy link
Member

@lmazuel lmazuel commented May 8, 2024

Fix #25640

We're not sure if we can auto-reopen the connection once it's closed, this can lead to leaky socket behavior. Therefore, this PR is just making explicit with a better error message that it's closed already.

Interesting thing is that if we call close while we never opened it, it shouldn't fail (it would be a bad breaking change), so the approach here is to track the open and the close event, for better acuracy.

I take the opportunity of this PR to fix the typing cast, that was ironically hidding bug #25640 (mypy tried to tell me, but I didn't listen at the time :p)

@johanste
Copy link
Member

johanste commented May 8, 2024

We should make sure we didn't break child clients/shared transports (e.g. storage container -> blob client)

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@xiangyan99
Copy link
Member

Now in the case of
open -> close -> open again,
we change the error type from AttributeError to ValueError, should we keep raising AttributeError and just update the error message?

@lmazuel
Copy link
Member Author

lmazuel commented May 13, 2024

Now in the case of open -> close -> open again, we change the error type from AttributeError to ValueError, should we keep raising AttributeError and just update the error message?

To me, an AttributeError from a client like that is a bug, so we're not breaking working code, but fixing an unexpected error. I don't see a scenario where a customer will write code to expect an AttributeError and recover from it (but of course everything is a breaking change :)). So I still vote to keep ValueError, but of course I'm open to various opinions.

Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Unless our architects have concerns on the error type change.

@lmazuel
Copy link
Member Author

lmazuel commented May 14, 2024

We should make sure we didn't break child clients/shared transports (e.g. storage container -> blob client)

We're passing the Storage tests, is that enough @johanste ?

@lmazuel lmazuel merged commit 70db3d9 into main Jun 4, 2024
214 of 224 checks passed
@lmazuel lmazuel deleted the fix_25640 branch June 4, 2024 22:32
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.

Result of query_entities of TableClient cannot be returned in with keyword successfully
5 participants