Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Connection Pool Refactor #182

Merged
merged 32 commits into from
Jul 31, 2019
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4e46ec9
Refactor from socket pool to stateful connection pool
trowski Jun 16, 2019
b09ced2
Move creating the socket to pool
trowski Jun 20, 2019
d042668
Mark busy until body is complete
trowski Jun 20, 2019
48239b5
Remove ConnectionInfo
trowski Jun 20, 2019
9a0afad
Decrease timeout in test
trowski Jun 20, 2019
766cadf
Forgot to remove ConnectionInfo class
trowski Jun 20, 2019
3fa3e34
Remove connection info from Response
trowski Jun 25, 2019
69abeb1
Support keep-alive
trowski Jun 28, 2019
fdaa26c
Use default or prior keep alive timeout if none is given
trowski Jul 12, 2019
80c88d8
Use new value component functions in amphp/http 1.3
trowski Jul 12, 2019
eab8ced
Do not replace promise with connection
trowski Jul 13, 2019
ac30d43
Move RequestWriter into Http1Connection
trowski Jul 13, 2019
4de37a9
Start of HTTP/2 connection
trowski Jul 13, 2019
e0a7960
Do not yield body part backpressure promise
trowski Jul 14, 2019
5e5590a
Fix settings issues causing disconnection
trowski Jul 14, 2019
69ab56e
Wait to ACK initial settings frame before sending first request
trowski Jul 14, 2019
60a32ca
Send null to parser after promise resolution
trowski Jul 15, 2019
1756b32
Add/normalize host header for HTTP/1.x; remove in HTTP/2
trowski Jul 26, 2019
69455ec
Port HTTP/2 improvements from http-server
trowski Jul 26, 2019
67624a7
Use hpack v2
trowski Jul 27, 2019
b70f00b
Remove unnecessary check
trowski Jul 27, 2019
ee40a6d
Require cancellation token in Connection
trowski Jul 27, 2019
b59c8e4
Fix rebase error
trowski Jul 27, 2019
783b63f
Fix pool reusing connections
trowski Jul 28, 2019
d1adfce
Various cleanup and organization
trowski Jul 29, 2019
675b75d
Normalize scheme and authority
trowski Jul 30, 2019
1582c44
Implement sending request body in HTTP/2
trowski Jul 30, 2019
7856054
Check content-length validity; do not remove chunked from transfer-en…
trowski Jul 30, 2019
1662985
Remove unused property of Http2Stream
trowski Jul 30, 2019
e721fcf
Simplify multiple connection handling
trowski Jul 30, 2019
7c5862a
Fix CS issues
trowski Jul 30, 2019
39b5a3b
Use null when creating stream if StringBody contains empty string
trowski Jul 30, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
"php": ">=7.1",
"amphp/amp": "^2",
"amphp/byte-stream": "^1.6",
"amphp/http": "^1.2",
"amphp/hpack": "^2",
"amphp/http": "^1.3",
"amphp/socket": "dev-master as 1.0",
"amphp/file": "^0.2 || ^0.3",
"kelunik/certificate": "^1.1",
Expand Down
2 changes: 1 addition & 1 deletion src/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface Client
*/
public function request(
Request $request,
CancellationToken $cancellation = null
?CancellationToken $cancellation = null
): Promise;

/**
Expand Down
12 changes: 6 additions & 6 deletions src/ClientBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@
namespace Amp\Http\Client;

use Amp\Http\Client\Internal\ApplicationInterceptorClient;
use Amp\Socket\SocketPool;
use Amp\Socket\UnlimitedSocketPool;
use Amp\Socket\Connector;
use Amp\Socket\DnsConnector;

final class ClientBuilder
{
private $socketPool;
private $connector;
private $networkInterceptors = [];
private $applicationInterceptors = [];

public function __construct(?SocketPool $socketPool = null)
public function __construct(?Connector $connector = null)
{
$this->socketPool = $socketPool ?? new UnlimitedSocketPool;
$this->connector = $connector ?? new DnsConnector;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fallback to connector() instead.

}

public function addNetworkInterceptor(NetworkInterceptor $networkInterceptor): self
Expand All @@ -33,7 +33,7 @@ public function addApplicationInterceptor(ApplicationInterceptor $applicationInt

public function build(): Client
{
$client = new SocketClient($this->socketPool);
$client = new SocketClient($this->connector);
foreach ($this->networkInterceptors as $networkInterceptor) {
$client->addNetworkInterceptor($networkInterceptor);
}
Expand Down
35 changes: 35 additions & 0 deletions src/Connection/Connection.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

namespace Amp\Http\Client\Connection;

use Amp\CancellationToken;
use Amp\Http\Client\Request;
use Amp\Http\Client\Response;
use Amp\Promise;
use Amp\Socket\SocketAddress;
use Amp\Socket\TlsInfo;

interface Connection
{
public const MAX_KEEP_ALIVE_TIMEOUT = 60;

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

public function isBusy(): bool;

public function close(): Promise;

public function onClose(callable $onClose): void;

public function getLocalAddress(): SocketAddress;

public function getRemoteAddress(): SocketAddress;

public function getTlsInfo(): ?TlsInfo;
}
18 changes: 18 additions & 0 deletions src/Connection/ConnectionPool.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

namespace Amp\Http\Client\Connection;

use Amp\CancellationToken;
use Amp\Http\Client\Request;
use Amp\Promise;

interface ConnectionPool
{
/**
* @param Request $request
* @param CancellationToken $token
*
* @return Promise<Connection>
*/
public function getConnection(Request $request, CancellationToken $token): Promise;
}
144 changes: 144 additions & 0 deletions src/Connection/DefaultConnectionPool.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
<?php

namespace Amp\Http\Client\Connection;

use Amp\ByteStream\StreamException;
use Amp\CancellationToken;
use Amp\CancelledException;
use Amp\Http\Client\Internal\CombinedCancellationToken;
use Amp\Http\Client\Request;
use Amp\Http\Client\SocketException;
use Amp\Http\Client\TimeoutException;
use Amp\Promise;
use Amp\Socket;
use Amp\Socket\ClientTlsContext;
use Amp\Socket\ConnectContext;
use Amp\Socket\Connector;
use Amp\Socket\EncryptableSocket;
use Amp\TimeoutCancellationToken;
use function Amp\call;

final class DefaultConnectionPool implements ConnectionPool
{
/** @var Connector */
private $connector;

/** @var Promise[][] */
private $connections = [];

public function __construct(?Connector $connector = null)
{
$this->connector = $connector ?? Socket\connector();
}

public function getConnection(Request $request, CancellationToken $cancellation): Promise
{
return call(function () use ($request, $cancellation) {
$uri = $request->getUri();
$isHttps = $uri->getScheme() === 'https';
$defaultPort = $isHttps ? 443 : 80;

$authority = $uri->getHost() . ':' . ($uri->getPort() ?: $defaultPort);
$key = $uri->getScheme() . '://' . $authority;

if (!empty($this->connections[$key])) {
foreach ($this->connections[$key] as $index => $connection) {
$connection = yield $connection;
\assert($connection instanceof Connection);

if (!$connection->isBusy()) {
return $connection;
}
}

++$index;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
$this->connections[$key] = [];
$index = 0;
}

$promise = $this->connections[$key][$index] = call(function () use ($request, $isHttps, $authority, $cancellation, $key, $index) {
kelunik marked this conversation as resolved.
Show resolved Hide resolved
$connectContext = new ConnectContext;

if ($isHttps) {
$tlsContext = ($connectContext->getTlsContext() ?? new ClientTlsContext($request->getUri()->getHost()))
->withApplicationLayerProtocols(['http/1.1'])
->withPeerCapturing();

if (\in_array('2.0', $request->getProtocolVersions(), true)) {
$tlsContext = $tlsContext->withApplicationLayerProtocols(['h2', 'http/1.1']);
}

$connectContext = $connectContext->withTlsContext($tlsContext);
}

try {
$checkoutCancellationToken = new CombinedCancellationToken($cancellation, new TimeoutCancellationToken($request->getTcpConnectTimeout()));

/** @var EncryptableSocket $socket */
$socket = yield $this->connector->connect('tcp://' . $authority, $connectContext, $checkoutCancellationToken);
} catch (Socket\ConnectException $e) {
throw new SocketException(\sprintf("Connection to '%s' failed", $authority), 0, $e);
} catch (CancelledException $e) {
// In case of a user cancellation request, throw the expected exception
$cancellation->throwIfRequested();

// Otherwise we ran into a timeout of our TimeoutCancellationToken
throw new TimeoutException(\sprintf("Connection to '%s' timed out, took longer than " . $request->getTcpConnectTimeout() . ' ms', $authority)); // don't pass $e
}

if ($isHttps) {
try {
$tlsState = $socket->getTlsState();
if ($tlsState === EncryptableSocket::TLS_STATE_DISABLED) {
$tlsCancellationToken = new CombinedCancellationToken($cancellation, new TimeoutCancellationToken($request->getTlsHandshakeTimeout()));
yield $socket->setupTls($tlsCancellationToken);
} elseif ($tlsState !== EncryptableSocket::TLS_STATE_ENABLED) {
throw new SocketException('Failed to setup TLS connection, connection was in an unexpected TLS state (' . $tlsState . ')');
}
} catch (StreamException $exception) {
throw new SocketException(\sprintf("Connection to '%s' closed during TLS handshake", $authority), 0, $exception);
} catch (CancelledException $e) {
// In case of a user cancellation request, throw the expected exception
$cancellation->throwIfRequested();

// Otherwise we ran into a timeout of our TimeoutCancellationToken
throw new TimeoutException(\sprintf("TLS handshake with '%s' @ '%s' timed out, took longer than " . $request->getTlsHandshakeTimeout() . ' ms', $authority, $socket->getRemoteAddress()->toString())); // don't pass $e
}
}

if ($isHttps && $socket->getTlsInfo()->getApplicationLayerProtocol() === 'h2') {
$connection = new Http2Connection($socket);
} else {
$connection = new Http1Connection($socket);
}

$connections = &$this->connections;
$connection->onClose(static function () use (&$connections, $key, $index) {
unset($connections[$key][$index]);

if (empty($connections[$key])) {
unset($connections[$key]);
}
});

return $connection;
});

$promise->onResolve(function (?\Throwable $exception) use ($key, $index): void {
if (!$exception) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Connection failed, remove from list of connections.
unset($this->connections[$key][$index]);

if (empty($this->connections[$key])) {
unset($this->connections[$key]);
}
});

return $promise;
});
}
}
Loading