You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by rschmitt <gi...@git.apache.org> on 2018/10/12 23:32:54 UTC
[GitHub] httpcomponents-core pull request #91: Interpret HTTP/2 window updates as inc...
GitHub user rschmitt opened a pull request:
https://github.com/apache/httpcomponents-core/pull/91
Interpret HTTP/2 window updates as increments
The `CapacityChannel#update(int)` method accepts a capacity *increment*, essentially allowing the caller to signal that a certain number of bytes has just been freed up. However, the HTTP/2 implementation interprets that value as the *total* buffer capacity, and thus drops all capacity
increments below a certain size, causing the window size to irreversibly shrink over time.
This change causes HTTP/2 capacity updates to be applied directly to the input window as increments. Since there exist `AsyncEntityConsumer` implementations (such as `StringAsyncEntityConsumer`) that supply a capacity of `Integer.MAX_VALUE`, the `updateWindow` method has been modified to tolerate integer overflow, instead of throwing an ArithmeticException. This is intended as an interim measure until the topic of capacity channel update management can be addressed more comprehensively.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rschmitt/httpcomponents-core master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpcomponents-core/pull/91.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #91
----
commit a5348255962438b8f2d9c19a58661e74bb2905cb
Author: Ryan Schmitt <ry...@...>
Date: 2018-10-12T23:19:17Z
Interpret HTTP/2 window updates as increments
The CapacityChannel#update(int) method accepts a capacity *increment*,
essentially allowing the caller to signal that a certain number of bytes
has just been freed up. However, the HTTP/2 implementation interprets
that value as the *total* buffer capacity, and thus drops all capacity
increments below a certain size, causing the window size to irreversibly
shrink over time.
This change causes HTTP/2 capacity updates to be applied directly to the
input window as increments. Since there exist AsyncEntityConsumer
implementations (such as StringAsyncEntityConsumer) that supply a
capacity of Integer.MAX_VALUE, the `updateWindow` method has been
modified to tolerate integer overflow, instead of throwing an
ArithmeticException. This is intended as an interim measure until the
topic of capacity channel update management can be addressed more
comprehensively.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt We cannot indiscriminately disregard update window overflow as that would be a direct violation of the HTTP/2 protocol. What we need to figure out is how to handle `CapacityChannel` increments gracefully in case they exceed the max limit.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core pull request #91: Interpret HTTP/2 window updates as inc...
Posted by rschmitt <gi...@git.apache.org>.
Github user rschmitt closed the pull request at:
https://github.com/apache/httpcomponents-core/pull/91
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by rschmitt <gi...@git.apache.org>.
Github user rschmitt commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@ok2c That's an improvement, but I still don't think we've gotten at the root of the problem yet. I don't think that a hardcoded capacity increment of `Integer.MAX_VALUE` is an appropriate way of signaling unlimited capacity, but it's much more difficult under the current design to do the right thing. Shouldn't it be simple for a consumer to signal `Integer.MAX_VALUE` (or some more reasonable value) as the *initial* capacity, and thereafter to signal capacity increments at the end of every `consume` call? (Another subtlety: when you think about it, claiming that you have an infinite buffer is not the same thing as claiming to always process consumed data instantaneously.)
I think a few interface changes could simplify things quite a bit:
1. The `CapacityChannel` should be supplied exactly once, before the `consume` and `streamEnd` methods are ever called; this could be an `init` method, which also returns the initial capacity of the consumer.
2. The consumer should always signal capacity increments by calling `CapacityChannel`; the consumer's caller should never ask for an increment directly.
I'll see how difficult it is to implement these changes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt The current API has been designed to work well with both HTTP/1.1 and HTTP/2.0. For instance, the initial capacity is out of control for individual HTTP/2 consumers. They must be prepared to consume at least the initial amount of data defined by HTTP/2 settings during the HTTP/2 protocol handshake. The capacity value returned by `#consume` method was necessary to make the same API work with HTTP/1.1 connections that do not even support a concept of data capacity. There is no easy way for HTTP/1.1 consumers to make the event handler aware of the fact that they are not able to process more data. The only alternative I can think of using different capacity interfaces for different HTTP protocol versions which would be terrible and hardly any simpler.
Please do go ahead and try to simplify the APIs as you deem fit, but the result might well end up being a different API complex in a different way.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by rschmitt <gi...@git.apache.org>.
Github user rschmitt commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@ok2c Your other option is to make minimum buffering requirements an explicit part of the contract, by doing one of the following:
1. Throwing an exception if an implementation of `initCapacity` returns an insufficient initial size
2. Passing your buffering requirements to the implementation as part of the `initCapacity` call and requiring the implementation to be able to buffer at least that much data
I don't have direct experience with the selector API in NIO, so it's tough for me to follow your description of the problem. Can't input event interest be set again as part of the `CapacityChannel#update` implementation (called back whenever data is read from the client's stream buffer)?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
> A buffering layer will be mandatory ..., but data only has to be buffered if the downstream consumer is not ready to process it;
@rschmitt Any notion of content buffering in the session layer makes me very, very uneasy. Once a chunk of message gets read from the socket channel, as far as the I/O selector is concerned the data has been fully consumed. It will no longer have any means of tracking that data. If the application layer stops reading data and clears interest in input events, the I/O selector will not be able to trigger read events on the associated channel, if the remaining message content has been buffered by the session. This problem has caused a lot of grief in HttpCore 4.x and its resolution involved a fair deal of ugly code. I was hoping to avoid having to do the same in HttpCore 5.x.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt The proposed interface does look simpler and more aesthetically pleasing. If we can get it to work there will be no questions asked.
> This is true, but I think it can be addressed simply by decoupling buffering and processing.
True, but I suppose not every data stream may need buffering. This change would make it mandatory if I understand it correctly.
> In reactive streaming, we signal exactly how much more data we can process; we don't explicitly tell the producer to stop, and the producer never sends us more data than we requested.
This does sound reasonable and this is where we should probably start.
When a particular change is too big and too disruptive what tends to work reasonably well is changing things one small thing at a time. Let's leave `AsyncDataConsumer` be for now but get rid of return type in AsyncDataConsumer#consume as the first step.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by rschmitt <gi...@git.apache.org>.
Github user rschmitt commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
> True, but I suppose not every data stream may need buffering. This change would make it mandatory if I understand it correctly.
A buffering *layer* will be mandatory (it has to go somewhere in any HTTP/2 implementation), but data only has to be buffered if the downstream consumer is not ready to process it; otherwise it can be passed directly downstream. Either way, these details can be encapsulated by the client. (We can further reduce buffering by only buffering the *initial* window, and letting the downstream consumer drive capacity updates directly after it has been consumed.)
> Let's leave `AsyncDataConsumer` be for now but get rid of return type in AsyncDataConsumer#consume as the first step.
I was actually thinking the same thing; this change by itself is a significant simplification without having to tackle the more difficult "lifecycle" changes (where `updateCapacity` is redesigned along the lines of `onSubscribe`). a5739b5dba9b10b357726602ede56406be2c8b69 looks good to me; I say ship it!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by rschmitt <gi...@git.apache.org>.
Github user rschmitt commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@ok2c I've been meaning to circle back on this discussion. I did a few hours of work on this problem and got the majority of the integration tests passing with my alternate backpressure interface, but the changes are definitely invasive and destabilizing, simply because of how widely implemented these interfaces are within the codebase; I think that changing these interfaces requires either more time or more experience with the codebase than I currently have. However, since these are *user*-facing interfaces that we'll be stuck with after release, I think we should seriously consider biting the bullet and making the changes.
The interface I've been working with is:
```java
public interface AsyncDataConsumer extends ResourceHolder {
int initCapacity(CapacityChannel capacityChannel);
void consume(ByteBuffer src) throws IOException;
void streamEnd(List<? extends Header> trailers) throws HttpException, IOException;
}
```
This interface implements the three points I suggested above, and is conceptually equivalent to the Reactive Streams API (where the `initCapacity` method is analogous to `onSubscribe`, and the `CapacityChannel` is the `Subscription`).
To address the points you made one by one:
> The current API has been designed to work well with both HTTP/1.1 and HTTP/2.0. For instance, the initial capacity is out of control for individual HTTP/2 consumers. They must be prepared to consume at least the initial amount of data defined by HTTP/2 settings during the HTTP/2 protocol handshake.
This is true, but I think it can be addressed simply by decoupling buffering and processing. Currently, if processing looks like:
`AbstractHttp2StreamMultiplexer -> Buffering processor`
We can simply refactor to:
`AbstractHttp2StreamMultiplexer -> Per-stream buffer -> AsyncDataConsumer`
This structure does a better job of separating internal concerns (per-stream buffering) and user concerns (the actual data and how it is processed), and makes it easier (fewer surprises) for users to implement the `AsyncDataConsumer` directly if they choose to do so.
> The capacity value returned by #consume method was necessary to make the same API work with HTTP/1.1 connections that do not even support the concept of data capacity.
I don't see how that follows. It's obviously true that HTTP/1.1 has no explicit windowing, but it's still an inherently asynchronous protocol, by virtue of the fact that it runs on top of TCP. You either read from the socket or abstain from reading from the socket, depending on your ability to process data (which is signaled via an asynchronous backpressure mechanism).
> There is no easy way for HTTP/1.1 consumers to make the event handler aware of the fact that they are not able to process any more input without data loss.
In reactive streaming, we signal exactly how much more data we *can* process; we don't explicitly tell the producer to *stop*, and the producer never sends us more data than we requested.
> The only alternative I can think of using different capacity interfaces for different HTTP protocol versions which would be terrible and hardly any simpler.
That would be terrible, but I think that protocol-specific buffering techniques (internal to the client) are all we really need here. The backpressure strategy I'm describing is one that I've implemented for both HTTP/1.1 and HTTP/2 on top of Netty; it works fine.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt How about this for a start a5739b5dba9b10b357726602ede56406be2c8b69? The change-set still needs polish. I saw `Http1IntegrationTest#testLargePost` fail once, there is still might be a race condition somewhere but that could be a start.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt How about this 277c628607e05c62d6fa9130a77ef7361aa6a49a?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt If the immediate problem has been resolved could you please close this PR and raise a new one once ready?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-core issue #91: Interpret HTTP/2 window updates as increments
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-core/pull/91
@rschmitt There are ways to approach the problem but I have not yet found an elegant solution. I need to step away from coding and force myself to write the documentation. At the moment the code is all yours. Do feel free to tweak the APIs as you see fit.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org