Fixes #6603 - HTTP/2 max local stream count exceeded #6639
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -131,7 +131,7 @@ public Pool(StrategyType strategyType, int maxEntries, boolean cache) | |||||||
| this.maxEntries = maxEntries; | ||||||||
| this.strategyType = strategyType; | ||||||||
| this.cache = cache ? new ThreadLocal<>() : null; | ||||||||
| this.nextIndex = strategyType == StrategyType.ROUND_ROBIN ? new AtomicInteger() : null; | ||||||||
| } | ||||||||
|
|
||||||||
| public int getReservedCount() | ||||||||
|
|
@@ -169,6 +169,14 @@ public final void setMaxMultiplex(int maxMultiplex) | |||||||
| if (maxMultiplex < 1) | ||||||||
| throw new IllegalArgumentException("Max multiplex must be >= 1"); | ||||||||
| this.maxMultiplex = maxMultiplex; | ||||||||
|
There was a problem hiding this comment. Should we rename the private field to make it clear it is the default:
Suggested change
Eitherway, this class now needs more javadoc describing how the maxMultiplex works There was a problem hiding this comment. We don't have the word "default" for a lot of other cases, e.g. There was a problem hiding this comment. But in other places we don't have the same fieldname on a subclass in the same file, but with a different meaning. |
||||||||
|
|
||||||||
| try (Locker.Lock l = locker.lock()) | ||||||||
| { | ||||||||
| if (closed) | ||||||||
| return; | ||||||||
|
|
||||||||
| entries.forEach(entry -> entry.setMaxMultiplex(maxMultiplex)); | ||||||||
gregw marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -507,15 +515,28 @@ public class Entry | |||||||
| // hi: positive=open/maxUsage counter; negative=closed; MIN_VALUE pending | ||||||||
| // lo: multiplexing counter | ||||||||
| private final AtomicBiInteger state; | ||||||||
| // The pooled item. This is not volatile as it is set once and then never changed. | ||||||||
| // Other threads accessing must check the state field above first, so a good before/after | ||||||||
| // relationship exists to make a memory barrier. | ||||||||
| private T pooled; | ||||||||
| private volatile int maxMultiplex; | ||||||||
|
There was a problem hiding this comment. I'm still wondering if we should store 2 values here:
Suggested change
We set localMaxMultiplex when we send a settings frame, we set remoteMaxMultiplex when we receive a settings frame. If we change settings, local changes first and then remote changes when we get the ack, otherwise remote changes first and local changes second. It will always be a hard error to have current greater than the localMaxMultiplex. If the localMaxMultiplex is less than the remoteMaxMultiplex then that doesn't matter, it is still and error have current greater than the local. If the localMaxMultiplex is greater than the remoteMaxMultiplex then we have to be more nuanced. It is an error for an remote created stream to exceed the remoteMaxMultiplex, but a locally created stream is allowed to let current exceed remoteMaxMultiplex but not localMaxMultiplex. I think we really need to deal with this type of dynamic max streams adjustment. We have already seen one example in the wild... others will follow even if it is a bad idea. There was a problem hiding this comment. Also this kind of exceeds my threshold for unused features in a base utility class. Perhaps we need to split There was a problem hiding this comment.
No, we don't. I don't think what you say is correct, although I understand the underlying idea of handling the case where a second incoming SETTINGS frame has a smaller There was a problem hiding this comment. Let's pair program on this tonight. I think it can be fixed in this PR. There was a problem hiding this comment. @sbordet As we discussed on hangout, we don't need to fix all the problems in this PR, but we should not add wrong things to the Pool. We should externalize the multiplexing checks as per my draft PR There was a problem hiding this comment. @gregw I second this goal. I'd be very happy if we managed to externalize the multiplexing out of the low-level That design also has adverse effects on performance as the delegation of the multiplexing accounting is delegated to a lower-level that the only thing it can do is to track it by using an atomic counter that's naturally going to be highly contended, i.e.: the HTTP2 pool is much slower when connections are multiplexed than when they're not because there's a ton of contention on the CAS loops around the multiplexing counters. If the multiplexing counter was tracked at the connection pool layer, we could add some contention-mitigating strategies. |
||||||||
|
|
||||||||
| Entry() | ||||||||
| { | ||||||||
| this.state = new AtomicBiInteger(Integer.MIN_VALUE, 0); | ||||||||
| this.maxMultiplex = Pool.this.maxMultiplex; | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| public int getMaxMultiplex() | ||||||||
| { | ||||||||
| return maxMultiplex; | ||||||||
| } | ||||||||
|
|
||||||||
| public void setMaxMultiplex(int maxMultiplex) | ||||||||
| { | ||||||||
| if (maxMultiplex < 1) | ||||||||
| throw new IllegalArgumentException("Max multiplex must be >= 1"); | ||||||||
| this.maxMultiplex = maxMultiplex; | ||||||||
| } | ||||||||
|
|
||||||||
| // for testing only | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is wrong. Setting the max multiplex for a connection is not the same as setting the default for the pool