Skip to content

THRIFT-6023: Add HTTP transport support to PHP cross-tests#3515

Open
sveneld wants to merge 1 commit into
apache:masterfrom
sveneld:THRIFT-6023
Open

THRIFT-6023: Add HTTP transport support to PHP cross-tests#3515
sveneld wants to merge 1 commit into
apache:masterfrom
sveneld:THRIFT-6023

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented May 21, 2026

Adds HTTP transport support to the PHP entry of the cross-test matrix. PHP was the only major-language entry stuck on buffered/framed while Python, Go, Node, C++, Java, Lua, Dart, JS, D, and Haskell already declare http and verify HTTP interop.

ServerTestServer.php detects --transport=http in CLI mode and pcntl_exec()s into PHP's built-in web server (php -S 127.0.0.1:$port TestServer.php). The same script re-enters under SAPI cli-server, reads the request via TPhpStream from php://input, dispatches it through the existing processor, and writes the response to php://output. The original -d flags from the launcher cmdline (e.g. -dextension=thrift_protocol.so) are preserved by parsing /proc/self/cmdline, so the accel extension and sockets stay loaded under the spawned server too. The cross-runner's socket.connect_ex readiness probe works because php -S TCP-binds the port immediately.

ClientTestClient.php gains an if ($MODE == 'http') branch that uses TPsrHttpClient (added in THRIFT-6010) against http://127.0.0.1:$port/. TPsrHttpClient buffers internally, so no TBufferedTransport/TFramedTransport wrapping.

tests.json — adds "http" to PHP server and client transports; bumps client timeout from 6s → 10s to absorb HTTP overhead.

composer.json — adds guzzlehttp/guzzle: ^7.8 to require-dev so the PSR-18 auto-discovery in TPsrHttpClient resolves a concrete HTTP client at cross-test time.

JIRA: THRIFT-6023. Draft until cross-test CI validates the new php↔php / php↔py3 / php↔go HTTP matrix cells.

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@kpumuk
Copy link
Copy Markdown
Member

kpumuk commented May 22, 2026

The last commit should hopefully be unnecessary with #3514.

For Java, the issue is that it is the only cross-test that actually verifies the one-way semantics, as in that HTTP server must return the response immediately, and then run the handler afterwards (so that the client is as fast as possible).

@sveneld
Copy link
Copy Markdown
Contributor Author

sveneld commented May 22, 2026

Thanks, i will update PR

@sveneld
Copy link
Copy Markdown
Contributor Author

sveneld commented May 22, 2026

@kpumuk Thanks — addressed both points in 025863b:

While I was in there also collapsed the if/elseif cascade in TestClient.php to two match expressions and dropped the dead TSocket assignment / unused $hosts/$logger.

sveneld added a commit to sveneld/thrift that referenced this pull request May 22, 2026
The cpp HTTP client cannot drain the empty oneway response that
THttpServer-style servers (including our HttpServer.php) emit; per the
maintainer note on apache#3515, THRIFT-6021 / apache#3514 fixes the cpp client side.
Until that lands, these 4 combinations stay in known_failures to keep
the matrix green; can be removed once apache#3514 is merged.
sveneld added a commit to sveneld/thrift that referenced this pull request May 29, 2026
The cpp HTTP client cannot drain the empty oneway response that
THttpServer-style servers (including our HttpServer.php) emit; per the
maintainer note on apache#3515, THRIFT-6021 / apache#3514 fixes the cpp client side.
Until that lands, these 4 combinations stay in known_failures to keep
the matrix green; can be removed once apache#3514 is merged.
@sveneld sveneld force-pushed the THRIFT-6023 branch 2 times, most recently from ffbaba0 to 0ef81ee Compare May 29, 2026 19:40
Client: php

PHP was the only major-language entry stuck on buffered/framed sockets
while Python, Go, Node, C++, Java, Lua, Dart, JS, D, and Haskell already
declared http in their cross-test transports.

Server (test/php/TestServer.php + HttpRouter.php + HttpServer.php):
- On --transport=http, TestServer pcntl_exec()s into 'php -S' with
  HttpRouter.php as the router. The chameleon HttpRouter dispatches on
  PHP_SAPI: cli launches php -S with itself as the per-request handler;
  cli-server invokes HttpServer.
- HttpServer reads php://input via TMemoryBuffer, peeks the message
  type, and for ONEWAY sends an empty HTTP 200 immediately while
  forking the handler so strict clients (Java) measure fast oneway
  latency. Sync path echoes the processor's response bytes.
- protocols.php centralises the protocol-name -> TProtocolFactory
  dispatch, shared between the socket entry point and the cli-server
  router.

Client (test/php/TestClient.php):
- Adds an http branch that uses TPsrHttpClient (THRIFT-6010) against
  http://127.0.0.1:$port/. Drops the duplicate makeProtocol() helper
  in favour of thrift_test_protocol_factory()->getProtocol(), removes
  the unused $host argv parsing, and normalises the file to PSR-12.

composer.json: adds guzzlehttp/guzzle ^7.8 to require-dev so PSR-18
discovery resolves a real client at cross-test time.

test/tests.json: adds 'http' to PHP server and client transports.

test/known_failures_Linux.json: lists php-cpp_*_http-ip (cpp THttpClient
does not handle Connection: close between calls — tracked as
THRIFT-6060) and nodejs-php_*_http-ip (Node http server mounts /test,
not /, matching the existing cpp-/d-/go-nodejs known failures).

Generated-by: Claude Opus 4.7
@sveneld sveneld marked this pull request as ready for review May 29, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants