michael-o commented on pull request #206: URL: https://github.com/apache/httpcomponents-core/pull/206#issuecomment-667709404
> > > > We already provide sane implementations with the name prefix Default. In this case it even covers the RFC for connection management which SHOULD (RFC) be the default behavior. > > In general I agree, however there are common scenarios where limiting upload speed to 8 MiB/second may not be considered sane. I'm worried that users may compare hc5 against other client implementations and find it to under-perform without realizing the ResonseOufOfOrderStrategy can be configured. I think most socket based java http client libraries have either no or limited support for out-of-order response checking, so it may be reasonable not to enable the check by default. On the other hand you're absolutely correct that users may also run into problems due to out order responses blocking request bodies from sending, and in that case it's not obvious how to opt into this feature. > All of that is to say, I think we could make an argument in either one being the default behavior, so we may want to avoid names that lock us into an approach in case there's overwhelming feedback from the community. I don't have a strong opinion on this, I only want call it out because it will be one of the hardest things to change once this feature is released. > > I like the sound of `NoResonseOufOfOrderStrategy` for the no-op implementation. I had planned to keep this change minimal and file another ticket to provide a no-op implementation, I'd be happy to add it to this PR if you prefer. Right, a safe implementation should not tremendously degrade perfomance. The default case should be using `Expect` header to avoid this altogether. They maybe also a case where both `Expect` and out of order detection can be chained. Consider that someone uploads a stream with chunked encoding while server says `100 Continue` somewhere in the middle the stream exceeds some internal server limit and the server closes the request prematurely. But I consider this to be an edge case. We have now the following options since RFC says `SHOULD` and blocking I/O imposes degregatation of performance: 1. Make `NoResponseOufOfOrderStrategy` or `IgnoringResponseOufOfOrderStrategy` default. 2. Rename `DefaultResponseOufOfOrderStrategy` to, as you have proposed, `SafeResponseOufOfOrderStrategy` or `MonitoringResponseOufOfOrderStrategy`. The monitoring implementation can receive another `int` arg which for how many chunks it should probe for an out of order response. Maybe it is for this implemtation sufficient to try 10 times and then if it hasn't failed yet, it passed server valadation and we can continue w/o any checks. WDYT? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
