Skip to content

deprecate ignoreUncleanSSLShutdown: Bool parameter #231

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

Open
weissi opened this issue May 28, 2020 · 5 comments
Open

deprecate ignoreUncleanSSLShutdown: Bool parameter #231

weissi opened this issue May 28, 2020 · 5 comments
Labels
kind/enhancement Improvements to existing feature.

Comments

@weissi
Copy link
Contributor

weissi commented May 28, 2020

We should probably deprecate the ignoreUncleanSSLShutdown parameter. AsyncHTTPClient knows when it can and when it cannot ignore an unclean SSL shutdown.

Basically if an .end was received and we have either a content-length OR a transfer-encoding: chunked header from the server, we can safely ignore. For HTTP/2 we can always ignore.

If we do have neither a content-length, nor a transfer-encoding: chunked header, then this should error.

@weissi
Copy link
Contributor Author

weissi commented May 28, 2020

@Lukasa does this sound right?

@Lukasa
Copy link
Collaborator

Lukasa commented May 28, 2020

Probably, with the caveat that in principle we may want users to be able to tolerate even the last case if they're sure they should. But frankly I think deprecating sounds good.

@weissi
Copy link
Contributor Author

weissi commented May 28, 2020

I agree with that. My thinking was that EOF framing over TLS should be super rare (am I right here?), and even rarer without CloseNotify, so I don't think that's a thing we need to support. And if we do then we can add a new one with a more scary name :). ignoreTrulyInsecureUncleanTLSShutdowns: Bool :)

@Lukasa
Copy link
Collaborator

Lukasa commented May 28, 2020

I believe it should be super rare, yes. As noted, I’m happy to deprecate.

@weissi
Copy link
Contributor Author

weissi commented Jun 12, 2020

For more context: #238 contains the full explanation why we don't need this parameter.

@artemredkin artemredkin added this to the 1.2.1 milestone Jun 13, 2020
@artemredkin artemredkin added the kind/enhancement Improvements to existing feature. label Jun 13, 2020
@artemredkin artemredkin modified the milestones: 1.2.1, 1.2.2 Aug 20, 2020
@artemredkin artemredkin modified the milestones: 1.2.2, 1.2.3 Nov 11, 2020
@weissi weissi removed this from the 1.2.3 milestone Nov 9, 2021
fabianfett added a commit that referenced this issue Nov 11, 2021
### Motivation

Fixes #238 and #231.

### Changes

- Extracted the unclean shutdown test from `HTTPClientTests` into their own file `HTTPClientUncleanSSLConnectionShutdownTests`
- Copy and pasted @weissi great explanation from #238 into the test file
- Removed property `ignoreUncleanSSLShutdown` everywhere

### Result

`ignoreUncleanSSLShutdown` on `HTTPClient.Configuration` is deprecated and ignored.

Co-authored-by: Johannes Weiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants