Fixes #6603 - HTTP/2 max local stream count exceeded #6639

sbordet · 2021-08-18T15:03:09Z

Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw

I've lots of questions... but at the very least we need more javadoc in Pool class

gregw · 2021-08-18T22:45:49Z

jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexConnectionPool.java

+    @Override
+    public void setMaxMultiplex(Connection connection, int maxMultiplex)
+    {
+        super.setMaxMultiplex(connection, maxMultiplex);
+    }


It is needed. The super method is protected, this is public.

Hmmmmm kind of proves my point that we need to separate out Pool and MultiplexedPool. It's not good to have "hidden" features on all pools that can be made public. I'd rather cut and paste the Pool class and not have all our other pools carrying around unused multiplex complexity.

gregw · 2021-08-18T22:49:31Z

jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java

@@ -785,6 +788,8 @@ protected IStream createRemoteStream(int streamId)
            int maxCount = getMaxRemoteStreams();
            if (maxCount >= 0 && remoteCount - remoteClosing >= maxCount)
            {
+                if (LOG.isDebugEnabled())
+                    LOG.debug("Could not create remote stream #{} for {}", streamId, this);


If you are keeping this debug, you may as well include maxCount, remoteCount & remoteClosing

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

gregw · 2021-08-18T23:04:26Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

@@ -169,6 +169,14 @@ public final void setMaxMultiplex(int maxMultiplex)
        if (maxMultiplex < 1)
            throw new IllegalArgumentException("Max multiplex must be >= 1");
        this.maxMultiplex = maxMultiplex;


Should we rename the private field to make it clear it is the default:

Suggested change

this.maxMultiplex = maxMultiplex;

this.defaultMaxMultiplex = maxMultiplex;

Eitherway, this class now needs more javadoc describing how the maxMultiplex works

We don't have the word "default" for a lot of other cases, e.g. ServerConnector.idleTimeout and EndPoint.idleTimeout, or the flow control windows got HTTP/2 ConnectionFactorys, so I would not add "default" here.
It is just the maxMultiplex of the Pool.

But in other places we don't have the same fieldname on a subclass in the same file, but with a different meaning.

gregw · 2021-08-18T23:11:24Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java


        Entry()
        {
            this.state = new AtomicBiInteger(Integer.MIN_VALUE, 0);
+            this.maxMultiplex = Pool.this.maxMultiplex;


Suggested change

this.maxMultiplex = Pool.this.maxMultiplex;

this.maxMultiplex = Pool.this.defaultMaxMultiplex;

gregw · 2021-08-18T23:19:04Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

        // 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;


I'm still wondering if we should store 2 values here:

Suggested change

private volatile int maxMultiplex;

private volatile int remoteMaxMultiplex;

private volatile int localMaxMultiplex;

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.

Also this kind of exceeds my threshold for unused features in a base utility class. Perhaps we need to split Pool into Pool and MultiplexPool?

We set localMaxMultiplex when we send a settings frame

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 max_concurrent_streams value.
However, I would handle this latter case in a different PR.

Let's pair program on this tonight. I think it can be fixed in this PR.

@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

@gregw I second this goal. I'd be very happy if we managed to externalize the multiplexing out of the low-level Pool as IMHO it was a design mistake.

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.

Updates after review. Updated the maxMultiplex mechanism to always work on Pool.Entry, rather than on Pool. Updated Pool javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2021-08-19T22:35:52Z

jetty-client/src/main/java/org/eclipse/jetty/client/ConnectionPool.java

+         */
+        default void setMaxMultiplex(Connection connection, int maxMultiplex)
+        {
+            setMaxMultiplex(maxMultiplex);


This implementation is wrong. Setting the max multiplex for a connection is not the same as setting the default for the pool

gregw · 2021-08-20T00:13:14Z

...ent-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java

@@ -52,6 +53,7 @@
    private final AtomicInteger sweeps = new AtomicInteger();
    private final Session session;
    private boolean recycleHttpChannels = true;
+    private int maxMultiplex = 1;


This is duplicated state... which is almost always bad. See below:

Argh!!! it id worse!!! This is triple state as it is also stored in Http2Session, but at least there is is kept correctly as the local and remote value.

gregw · 2021-08-20T00:14:04Z

...ent-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java

@@ -74,6 +76,16 @@ public void setRecycleHttpChannels(boolean recycleHttpChannels)
        this.recycleHttpChannels = recycleHttpChannels;
    }

+    public int getMaxMultiplex()
+    {
+        return maxMultiplex;


Avoid duplicate state:

Suggested change

return maxMultiplex;

Object attachment = getAttachment();

return (attachment instanceof ConnectionPool.Multiplexable)

? ((ConnectionPool.Multiplexable)attachment).getMaxMultiplex()

: 0;

@sbordet I think this could at least remove some state duplication.... but might not be needed if we can delegate all the way down to HTTP2Session.

gregw · 2021-08-20T00:45:13Z

I think the fundamental problem here is that we are duplicating state. The Http2Session correctly maintains the local and remote max streams, but we then duplicate this as a single value in the Pool abstraction.

I think we need to abstract the multiplexing support from the Pool so that it can access the state correctly held in Http2Session.

Use interface

sbordet · 2021-08-26T15:07:28Z

@gregw, @lorban and I gave it a go at trying to move the multiplex feature out of Pool but that means that we need to move out the Pool.StrategyType too, and also provide (again) a way to access the Pool by index (as now the strategy of what Pool.Entry to access is driven from outside).

FTR, moving out usageCount would be trivial, and tracked by #6669.

I think the best solution would be to keep multiplexing in Pool, as the cost of moving it out is not compensating the advantages, which may amount in just some less contention on an atomic counter (at the cost of dropping some other feature like the strategies), but very likely will result in the same contention, just elsewhere (if we want to keep the strategies).

About the thought bubble of having the multiplex check externalized, it may be a good idea, but A) it is not necessary to fix this particular issue, and B) I don't think it will solve the general problem anyway, as I can still see a race where a thread passes the check, but by the time it arrives to create a local stream representing the request to send, the value may have been changed with the arrival of a SETTINGS frame.

I opened #6670 about the decrement of max_concurrent_streams.

I think this PR is good as is, so I refreshed the review status.

gregw · 2021-08-26T21:54:04Z

@sbordet Externalizing the multiplex check does work. The race you describe definitely does exist, but that's the second issue that I said we did not want to fix in this PR. At least if the multiplex check is externalized then that race can be fixed by putting all the logic/state around what is the max into a single state that is aware of settings received, sent and messages queued (ie the session).

That race will never EVER be solved inside the pool so moving more multiplexing logic inside the pool is a step in the wrong direction.

As @lorban says, there is a cost in multiplexing support that we are having on all our pool usages. So perhaps we should just split Pool into Pool and MultiPool (it would be nice if the later extended the former, but whilst I think that is possible, it is not necessary). But even then, I would argue that the max streams logic should be external to the pool as per my proposed PR, as it is only in the Session class that the race of changing settings can be solved (but not necessarily by this PR).

Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updates after review. Updated the maxMultiplex mechanism to always work on Pool.Entry, rather than on Pool. Updated Pool javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Incorporated changes from #6648. Now the maxMultiple value is pulled from its primary value, i.e. HTTP2Session.maxLocalStreams, rather than being set in multiple places. Deprecated usages of maxMultiplex and maxUsageCount. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet · 2021-08-27T20:41:10Z

@gregw @lorban I have incorporate the changes from #6648 and rebased so the changes in this PR are now only fixing the original issue. Please re-review.

The only glitch is that it also deprecates maxUsage, which should be addressed in a different issue.
It can be easily reverted, although its deprecation is quite similar to that of maxMultiplex.

gregw · 2021-08-30T00:01:21Z

jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java

+        if (maxMultiplex >= 0 || maxUsage >= 0)
+            return new MultiEntry();
+        return new MonoEntry();


Having this as a dynamic decision does have a small cost (hopefully which the JIT can optimize out). But it is necessary to maintain the current API. Ultimately once we remove the deprecated methods from the base pool, then this decision can be made in a overriden method in the class that also provides the MultiEntry. So I think this is a good compromise with the current API.

@lorban can you do some performance tests to see if the NonoEntry based pool is faster than the previous for things like ByteBufferPool. I'm not expecting much, as it still hits an atomic int about as much as the previous atomic long... but it does avoid accessing the volatile max fields, so that could be a small win.

I can try benchmarking Pool again to make sure how this change affects perf, but I already have some concerns with this newEntry method.

Now that there can be multiple implementations of Entry, all try* calls have become virtual. They used to be monomorphic, this implementation makes them bi-morphic as there are by default 2 classes extending from Entry. But anyone extending Pool to override this method will make those try* methods mega-morphic.

Since tryAcquire is on the fast path (it's in the hot loop of acquire) this change introduces an easy way to silently harm performance as there can now be a mega-morphic call on the fast path. IIRC this is the reason why the pool's strategy was written the way it was because the original proposal was using multiple implementations and was measurably slower.

gregw

LGTM, but then I had a hand in much of it... so I won't approve.

gregw · 2021-08-30T00:05:28Z

...ransport/src/main/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2.java

@@ -60,7 +60,7 @@ public HttpClientTransportOverHTTP2(HTTP2Client client)
        setConnectionPoolFactory(destination ->
        {
            HttpClient httpClient = getHttpClient();
-            return new MultiplexConnectionPool(destination, httpClient.getMaxConnectionsPerDestination(), destination, httpClient.getMaxRequestsQueuedPerDestination());
+            return new MultiplexConnectionPool(destination, httpClient.getMaxConnectionsPerDestination(), destination, 1);


I'm assuming the 1 is there until overridden by the settings frame? Eitherway a comment would be good explaining why 1.

Added comment as suggested in review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban

The newEntry method should be made private and commented to make sure we never mistakenly add a 3rd Entry implementation.

Made newEntry() private to avoid creation of more than 2 Entry subclasses. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis. Updated Pool javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com> Co-authored-by: Greg Wilkins <gregw@webtide.com> (cherry picked from commit 525fcb3)

* Fixes #6603 - HTTP/2 max local stream count exceeded (#6639) Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis. Updated Pool javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com> Signed-off-by: Greg Wilkins <gregw@webtide.com> Co-authored-by: Greg Wilkins <gregw@webtide.com> (cherry picked from commit 525fcb3)

Fixes #6603 - HTTP/2 max local stream count exceeded

6ea6bda

Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from gregw and lorban August 18, 2021 15:03

sbordet added this to In progress in Jetty 9.4.44 FROZEN via automation Aug 18, 2021

sbordet linked an issue Aug 18, 2021 that may be closed by this pull request

HTTP/2 max local stream count exceeded #6603

Closed

gregw requested changes Aug 18, 2021

View reviewed changes

Jetty 9.4.44 FROZEN automation moved this from In progress to Review in progress Aug 18, 2021

Issue #6603 - HTTP/2 max local stream count exceeded

cb102d9

Updates after review. Updated the maxMultiplex mechanism to always work on Pool.Entry, rather than on Pool. Updated Pool javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw reviewed Aug 19, 2021

View reviewed changes

gregw reviewed Aug 20, 2021

View reviewed changes

gregw added 3 commits August 20, 2021 11:39

Thought bubble on abstracting out multiplexing from the pool

a1297ca

revert to the original naming

e4d1fb9

Review suggestion

5834e5f

Use interface

sbordet requested a review from gregw August 26, 2021 15:07

gregw and others added 6 commits August 27, 2021 12:36

Work In Progress

7b78b90

Work In Progress

c3a19a6

Fixes #6603 - HTTP/2 max local stream count exceeded

216616a

Made MAX_CONCURRENT_STREAMS setting work on a per-connection basis. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Issue #6603 - HTTP/2 max local stream count exceeded

9caf6fa

Updates after review. Updated the maxMultiplex mechanism to always work on Pool.Entry, rather than on Pool. Updated Pool javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Issue #6603 - HTTP/2 max local stream count exceeded

e27f192

Fixed javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet force-pushed the jetty-9.4.x-6603-max-concurrent-streams-exceeded branch from 95265bf to 7733d0f Compare August 27, 2021 20:38

gregw reviewed Aug 30, 2021

View reviewed changes

Issue #6603 - HTTP/2 max local stream count exceeded

97f4ef3

Added comment as suggested in review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from gregw August 30, 2021 08:10

lorban requested changes Aug 30, 2021

View reviewed changes

Issue #6603 - HTTP/2 max local stream count exceeded

aa3e7de

Made newEntry() private to avoid creation of more than 2 Entry subclasses. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from lorban August 30, 2021 13:50

lorban approved these changes Aug 30, 2021

View reviewed changes

sbordet merged commit 525fcb3 into jetty-9.4.x Aug 30, 2021

Jetty 9.4.44 FROZEN automation moved this from Review in progress to Done Aug 30, 2021

sbordet deleted the jetty-9.4.x-6603-max-concurrent-streams-exceeded branch August 30, 2021 14:00

sbordet mentioned this pull request Aug 30, 2021

Fixes #6603 - HTTP/2 max local stream count exceeded (#6639) #6682

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6603 - HTTP/2 max local stream count exceeded #6639

Fixes #6603 - HTTP/2 max local stream count exceeded #6639

sbordet commented Aug 18, 2021

gregw left a comment

gregw Aug 18, 2021

sbordet Aug 19, 2021

gregw Aug 19, 2021

gregw Aug 18, 2021

sbordet Aug 19, 2021

gregw Aug 18, 2021

sbordet Aug 19, 2021

gregw Aug 19, 2021

gregw Aug 18, 2021

gregw Aug 18, 2021

gregw Aug 18, 2021

sbordet Aug 19, 2021

gregw Aug 19, 2021

gregw Aug 20, 2021

lorban Aug 23, 2021

gregw Aug 19, 2021

gregw Aug 20, 2021

gregw Aug 20, 2021

gregw Aug 20, 2021

gregw Aug 20, 2021

gregw commented Aug 20, 2021

sbordet commented Aug 26, 2021

gregw commented Aug 26, 2021

sbordet commented Aug 27, 2021

gregw Aug 30, 2021

lorban Aug 30, 2021

gregw left a comment

gregw Aug 30, 2021

sbordet Aug 30, 2021

lorban left a comment

	this.maxMultiplex = maxMultiplex;
	this.defaultMaxMultiplex = maxMultiplex;

	this.maxMultiplex = Pool.this.maxMultiplex;
	this.maxMultiplex = Pool.this.defaultMaxMultiplex;

	private volatile int maxMultiplex;
	private volatile int remoteMaxMultiplex;
	private volatile int localMaxMultiplex;

-        return maxMultiplex;
+        Object attachment = getAttachment();
+        return (attachment instanceof ConnectionPool.Multiplexable)
+            ? ((ConnectionPool.Multiplexable)attachment).getMaxMultiplex()
+            : 0;

Fixes #6603 - HTTP/2 max local stream count exceeded #6639

Fixes #6603 - HTTP/2 max local stream count exceeded #6639

Conversation

sbordet commented Aug 18, 2021

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Aug 20, 2021

sbordet commented Aug 26, 2021

gregw commented Aug 26, 2021

sbordet commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment