[WIP] Connection Pool Refactor #182

trowski · 2019-06-17T04:53:42Z

Currently a WIP. I still need to refine the structure a bit more and add some needed functionality to how the pool handles returning connections (e.g.: keep-alive vs. close connections).

kelunik · 2019-06-17T06:20:58Z

src/ClientBuilder.php

    {
-        $this->socketPool = $socketPool ?? new UnlimitedSocketPool;
+        $this->connector = $connector ?? new DnsConnector;


This should fallback to connector() instead.

kelunik · 2019-06-17T06:22:33Z

src/Connection/DefaultConnectionPool.php

+    public function createConnection(Socket $socket): Connection
+    {
+        $connection = new Http1Connection($socket);
+        $this->connections[$socket->getRemoteAddress()->toString()] = $connection;


I wanted to say this only allows one concurrent connection per host, but actually this works, because it has the port in it.

So it allows one concurrent connection per host per port, which isn't really enough for HTTP/1.x, but works fine for HTTP/2. This is one of the areas where I'm not exactly sure how I want to approach the design.

Ugh, well, right. I think I'd add Connection::isBusy and ConnectionPool chooses the next connection then or creates a new one if none is available. This also allows limiting the number of requests per HTTP/2 connection then, so the connection manager creates a separate HTTP/2 connection at some point.

The major issue here is that we can't use the remote address as key, because different sites might be hosted on the same IP, but due to SNI there might be different TLS certificates served.

kelunik · 2019-06-20T09:44:39Z

src/Connection/DefaultConnectionPool.php


-        $address = $uri->getHost();
-        $port = $uri->getPort();
+            $authority = $request->getUri()->getHost() . ':' . ($request->getUri()->getPort() ?: $defaultPort);


We should probably error here if we didn't get an absolute URI.

kelunik · 2019-06-20T09:49:43Z

src/Connection/DefaultConnectionPool.php

-        if ($port !== null) {
-            $address .= ':' . $port;
-        }
+            if (isset($this->connections[$authority])) {


We should also include the scheme in the key here, there are very rare cases where requests with http and https are supported on the same port.

kelunik · 2019-06-20T09:52:34Z

src/Connection/Http1Connection.php

-                return yield from $this->doRead($request, $cancellation, $readingCancellation, $completionDeferred);
+                $response = yield from $this->doRead($request, $cancellation, $readingCancellation, $completionDeferred);
+
+                $this->busy = false;


This is too early. You need to subscribe to the completion promise in Response instead, because at this point we have only read the header, not the body.

This was intentional, since another request can be made while a prior response body is being streamed… but that could cause another request to be severely delayed, so I'll use the completion promise instead.

As far as I know no browser supports pipelining, so we probably shouldn't either.

We do in http-server (because we should to comply with the RFC I think…), but in the client, I agree.

Yes, for http-server we need to support it.

kelunik · 2019-06-21T05:53:22Z

src/Response.php

@@ -33,7 +36,7 @@ public function __construct(
        array $headers,
        InputStream $body,
        Request $request,
-        ConnectionInfo $connectionInfo,
+        Connection $connection,


Response shouldn't have a reference to the connection itself.

I suppose not… I was hoping to just drop ConnectionInfo, but maybe I'll have to keep it after all. Does it make sense to pass a Connection to NetworkInterceptor too?

NetworkInterceptor can receive Connection, that's fine. We'll have to keep ConnectionInfo for Response or remove support for it in Response and if you want to access it, you have to implement a NetworkInterceptor.

I think I like the second option, since 99.9% of the time you don't care about that information.

src/Connection/DefaultConnectionPool.php

kelunik · 2019-06-28T20:05:30Z

src/Connection/DefaultConnectionPool.php

+                        unset($this->connections[$key]);
+                    }
+                    return;
+                }


We only push the connection to the array two lines below, so there shouldn't be any entry here to delete.

The promise is pushed to the array when it's created, so that needs to be deleted.

src/Connection/Http1Connection.php

kelunik · 2019-06-28T20:11:19Z

src/Connection/Http1Connection.php

+            return 0;
+        }
+
+        if (\strcasecmp($responseConnHeader, 'keep-alive')) {


Should be checking against close, too, as keep-alive is the default for HTTP/1.1.

If the value is anything other than 'keep-alive' the connection will be closed.

src/Connection/Http1Connection.php

src/Connection/DefaultConnectionPool.php

kelunik · 2019-07-14T18:27:09Z

src/Connection/Http2Connection.php

@@ -75,6 +75,9 @@ final class Http2Connection implements Connection
    public const INADEQUATE_SECURITY = 0xc;
    public const HTTP_1_1_REQUIRED = 0xd;

+    public const DEFAULT_MAX_HEADER_SIZE = 1 << 20;


1MB for headers is a bit much, no?

Probably. We'll have to look at if and how we want these settings to be configured and pick some defaults.

Should not matter too much as a client. TBH, if you do random requests to not-somewhat-known sites, then you need sanity checks at every layer.

A WIP at this point, but it sort of works.

Duplicate pseudo-headers are checked above.

kelunik · 2019-07-27T14:31:32Z

src/Connection/Connection.php

+     *
+     * @return Promise<Response>
+     */
+    public function request(Request $request, ?CancellationToken $token = null): Promise;


CancellationToken isn't optional in ConnectionPool, so probably can be required here, too?

Yeah, the Client object should take care of creating one.

kelunik · 2019-07-27T14:38:36Z

src/Connection/DefaultConnectionPool.php

+                    }
+                }
+
+                ++$index;


This isn't safe. Suppose we have 3 concurrent connections which all allow just one request. Due to the yielding above, the second and third both have $index = 0 and hang in the yield until the connection is established. Both push to $index = 1 then.

Ah, right, since the new connections won't be added to the list used by foreach. Hmm… might have to use a queue or something where that would be the case.

I'd just do a keyless push and query the last key.

Also actually use it in Http2Connection. :-)

src/Response.php

src/Connection/Http2Connection.php

src/SocketClient.php

…coding header

kelunik reviewed Jun 17, 2019

View reviewed changes

kelunik reviewed Jun 20, 2019

View reviewed changes

kelunik reviewed Jun 21, 2019

View reviewed changes

kelunik reviewed Jun 28, 2019

View reviewed changes

kelunik reviewed Jul 13, 2019

View reviewed changes

src/Connection/DefaultConnectionPool.php Outdated Show resolved Hide resolved

kelunik reviewed Jul 14, 2019

View reviewed changes

trowski added 21 commits July 27, 2019 09:26

Refactor from socket pool to stateful connection pool

4e46ec9

Move creating the socket to pool

b09ced2

Mark busy until body is complete

d042668

Remove ConnectionInfo

48239b5

Decrease timeout in test

9a0afad

Forgot to remove ConnectionInfo class

766cadf

Remove connection info from Response

3fa3e34

Support keep-alive

69abeb1

Use default or prior keep alive timeout if none is given

fdaa26c

Use new value component functions in amphp/http 1.3

80c88d8

Do not replace promise with connection

eab8ced

Move RequestWriter into Http1Connection

ac30d43

Start of HTTP/2 connection

4de37a9

A WIP at this point, but it sort of works.

Do not yield body part backpressure promise

e0a7960

Fix settings issues causing disconnection

5e5590a

Wait to ACK initial settings frame before sending first request

69ab56e

Send null to parser after promise resolution

60a32ca

Add/normalize host header for HTTP/1.x; remove in HTTP/2

1756b32

Port HTTP/2 improvements from http-server

69455ec

Use hpack v2

67624a7

Remove unnecessary check

b70f00b

Duplicate pseudo-headers are checked above.

kelunik reviewed Jul 27, 2019

View reviewed changes

trowski force-pushed the connection-refactor branch from bf2f820 to b70f00b Compare July 27, 2019 14:34

kelunik reviewed Jul 27, 2019

View reviewed changes

trowski added 2 commits July 27, 2019 12:08

Require cancellation token in Connection

ee40a6d

Also actually use it in Http2Connection. :-)

Fix rebase error

b59c8e4

kelunik reviewed Jul 28, 2019

View reviewed changes

src/Response.php Outdated Show resolved Hide resolved

Fix pool reusing connections

783b63f

kelunik reviewed Jul 28, 2019

View reviewed changes

src/Connection/Http2Connection.php Outdated Show resolved Hide resolved

src/SocketClient.php Outdated Show resolved Hide resolved

trowski added 8 commits July 29, 2019 13:40

Various cleanup and organization

d1adfce

Normalize scheme and authority

675b75d

Implement sending request body in HTTP/2

1582c44

Check content-length validity; do not remove chunked from transfer-en…

7856054

…coding header

Remove unused property of Http2Stream

1662985

Simplify multiple connection handling

e721fcf

Fix CS issues

7c5862a

Use null when creating stream if StringBody contains empty string

39b5a3b

trowski merged commit 39b5a3b into master Jul 31, 2019

trowski temporarily deployed to github-pages July 31, 2019 20:06 Inactive

kelunik deleted the connection-refactor branch July 31, 2019 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Connection Pool Refactor #182

[WIP] Connection Pool Refactor #182

trowski commented Jun 17, 2019

kelunik Jun 17, 2019

kelunik Jun 17, 2019

trowski Jun 17, 2019

kelunik Jun 17, 2019

kelunik Jun 20, 2019

kelunik Jun 20, 2019

kelunik Jun 20, 2019

trowski Jun 20, 2019

kelunik Jun 20, 2019

trowski Jun 20, 2019

kelunik Jun 21, 2019

kelunik Jun 21, 2019

trowski Jun 21, 2019

kelunik Jun 21, 2019

trowski Jun 21, 2019

kelunik Jun 28, 2019

trowski Jun 28, 2019

kelunik Jun 28, 2019

trowski Jun 28, 2019

kelunik Jul 14, 2019

trowski Jul 15, 2019

bwoebi Jul 15, 2019

kelunik Jul 27, 2019

trowski Jul 27, 2019

kelunik Jul 27, 2019

trowski Jul 27, 2019

kelunik Jul 27, 2019

+                                  }
+                              }
+                              ++$index;

[WIP] Connection Pool Refactor #182

[WIP] Connection Pool Refactor #182

Conversation

trowski commented Jun 17, 2019

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

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