amphp / amphp/http-server

SocketHttpServer never closes the client when the HTTP driver returns normally (fd leak, can pin a core at 100%)

Open Beginner friendly
#390 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
1.3k
Forks
103
PR merge metrics
No merged PRs in 30d

Description

Summary

SocketHttpServer::handleClient() only closes the client on three paths: a failed TLS handshake, a not-yet-started server, or an exception reaching the catch block. When $driver->handleClient() returns normally, the finally block just unsets the driver from the map and the socket is never closed:

try {
    $driver->handleClient($client, $socket, $socket);
} finally {
    unset($this->drivers[$id]);   // <- no $client->close()
}

Nothing releases the descriptor afterwards. Neither ReadableResourceStream::close() nor WritableResourceStream::close() calls fclose() on a socket — each does stream_socket_shutdown(SHUT_RD/SHUT_WR) and drops its own reference, so the fd is freed only once both halves free it and the refcount reaches zero. With no close() at all, the descriptor and its event-loop watcher are retained for the lifetime of the process.

Version

amphp/http-server 3.4.5 (also present in 3.x main as far as I can tell), amphp/byte-stream 2.1.2, PHP 8.4.

Reproduction

Deterministic — no timing dependency. A trivial HttpDriver that returns from handleClient() reproduces the same control flow a real driver takes at end-of-connection:

$noopDriver = new class implements HttpDriver {
    public ?Client $seen = null;
    public function handleClient(Client $client, ReadableStream $r, WritableStream $w): void {
        $this->seen = $client;   // return normally, as at end-of-connection
    }
    public function getPendingRequestCount(): int { return 0; }
    public function getApplicationLayerProtocols(): array { return []; }
    public function stop(): void {}
};

$server = new SocketHttpServer(new NullLogger(), new ResourceServerSocketFactory(),
    new SocketClientFactory(new NullLogger()), httpDriverFactory: $factoryReturning($noopDriver));
$server->expose(SocketAddress\fromString('127.0.0.1:0'));
$server->start($requestHandler, new DefaultErrorHandler());

$client = Socket\connect($server->getServers()[0]->getAddress()->toString());
$client->write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
\Amp\delay(0.3);
$client->close();
\Amp\delay(0.3);

var_dump($noopDriver->seen->isClosed());   // expected true

Output:

driver received a client:            yes
client closed after driver returned: NO
RESULT: LEAKED - SocketHttpServer never closed the client
Impact

Beyond the descriptor leak, whether the leak is merely wasteful or actively burns a core depends on the write queue at the moment of abandonment. In WritableResourceStream's watcher finally:

  • write queue empty → EventLoop::disable() → idle leak, no CPU cost
  • write queue non-empty → the watcher is left enabled

A dead socket is permanently "writable" as far as epoll is concerned, so in the second case the watcher retries a failing write forever. Measured at roughly 30k EPIPE writes/second, per leaked descriptor, indefinitely.

In a production deployment this surfaced as two cluster workers pinned at 100% CPU continuously for 19 days, holding 35 and 37 orphaned descriptors between them. The real-world trigger is a peer that RSTs a connection sitting idle in HTTP/1.1 keep-alive — i.e. ordinary internet scanner traffic against a public listener. Roughly 60% of such connections leaked in our testing.

Suggested fix
             try {
                 $driver->handleClient($client, $socket, $socket);
             } finally {
+                $client->close();
                 unset($this->drivers[$id]);
             }

With that applied the reproduction above prints client closed after driver returned: yes, and the production symptom disappears: 15 runs of the triggering sequence produce zero orphaned descriptors and 0% idle CPU.

For what it's worth, this is what Amp\Cluster users are most exposed to, and it is also worth noting that at least one downstream project which modelled its own socket server on this method (Flyokai\DataService\DataServer\SocketDataServer::handleClient()) does call $client->close() in the equivalent finally — which is why its non-HTTP listener never exhibited the leak while the HTTP one did.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at SocketHttpServer::handleClient() and run the deterministic reproduction described in the issue, focusing on the normal-return path from HttpDriver::handleClient(). Verify that the client is closed after the driver returns and that repeated runs no longer retain orphaned descriptors or produce the reported busy-writer behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend, networking
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.