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

Remove Exception instance for TLSError, bump to 1.8.0 #457

Closed

Conversation

andrewthad
Copy link
Contributor

Stop throwing TLSError and IOException to make it the exceptions thrown by this library more predicatable.

Resolves #456

Stop throwing TLSError and IOException to make it the exceptions thrown
by this library more predicatable.
@andrewthad
Copy link
Contributor Author

Also, I think we should let this sit for a while to see if there is any feedback about how this may negatively affect users.

@kazu-yamamoto kazu-yamamoto self-requested a review August 1, 2023 00:35
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

Looks lovely.
I will release v1.7.1 first then merge this PR into master.

kazu-yamamoto added a commit to kazu-yamamoto/hs-tls that referenced this pull request Aug 1, 2023
@kazu-yamamoto
Copy link
Collaborator

Merged.
But I found a problem.
http-client-tls cannot be build since TLSError is not Exception anymore.
I will revert this part.

@kazu-yamamoto
Copy link
Collaborator

Done.

@kazu-yamamoto
Copy link
Collaborator

A test case of quic gets failed.
I will take care of this.

@andrewthad
Copy link
Contributor Author

In the longer term, http-client-tls should stop catching exceptions of type TLSError since they are not thrown anymore. But this could be part of a slower plan that happens over the course of a year.

My fear is that, if the Exception instance for TLSError remains available, someone will accidentally start throwing TLSError again. I guess we could add a comment that says something like: "TLSError has an Exception instance for compatibility reasons, but nothing in this library throws TLSError, and the instance will be removed in the future."

@kazu-yamamoto
Copy link
Collaborator

I agree.
I will revert the latest commit and send a PR to http-client-tls.
But it would take time because I'm now trying to understand the whole story.

@kazu-yamamoto
Copy link
Collaborator

For quic, it seems to me that the following commit solves the issue:
kazu-yamamoto/quic@edd1ab3

@kazu-yamamoto
Copy link
Collaborator

@andrewthad Please give a look at kazu-yamamoto/http-client@73d1a4e.

If this is OK with you, I will send a PR to http-client.

@kazu-yamamoto
Copy link
Collaborator

@andrewthad Gentle ping.

@andrewthad
Copy link
Contributor Author

Sorry, I hadn't noticed the command asking me to look at the http-client commit. Yes, guarding the catching of TLSError like that with MIN_VERSION_tls looks right to me.

@kazu-yamamoto
Copy link
Collaborator

@andrewthad New versions of http-client-tls, warp-tls and quic are on Hackage.
I'm going to release tls v1.8.0 tomorrow.

@kazu-yamamoto
Copy link
Collaborator

v1.8.0 has been released.

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.

Guidance around exceptions
2 participants