-
Notifications
You must be signed in to change notification settings - Fork 6
Run conformance tests with HTTP/3 #151
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import argparse | ||
| import asyncio | ||
| import contextlib | ||
| import os | ||
| import re | ||
| import signal | ||
|
|
@@ -10,13 +11,15 @@ | |
| import sys | ||
| import time | ||
| from contextlib import ExitStack, closing | ||
| from pathlib import Path | ||
| from tempfile import NamedTemporaryFile | ||
| from typing import TYPE_CHECKING, Literal, TypeVar, get_args | ||
|
|
||
| # Needs to run before importing from connectrpc | ||
| import _cov_embed # noqa: F401 | ||
| from _util import create_standard_streams | ||
| from gen.connectrpc.conformance.v1.config_pb2 import Code as ConformanceCode | ||
| from gen.connectrpc.conformance.v1.config_pb2 import HTTPVersion | ||
| from gen.connectrpc.conformance.v1.server_compat_pb2 import ( | ||
| ServerCompatRequest, | ||
| ServerCompatResponse, | ||
|
|
@@ -43,6 +46,7 @@ | |
| UnaryResponseDefinition, | ||
| ) | ||
| from google.protobuf.any_pb2 import Any | ||
| from pyvoy import PyvoyServer | ||
|
|
||
| from connectrpc.code import Code | ||
| from connectrpc.compression.brotli import BrotliCompression | ||
|
|
@@ -635,40 +639,47 @@ async def serve_pyvoy( | |
| cafile: str | None, | ||
| port_future: asyncio.Future[int], | ||
| ): | ||
| args = ["--port=0"] | ||
| if certfile: | ||
| args.append(f"--tls-cert={certfile}") | ||
| if keyfile: | ||
| args.append(f"--tls-key={keyfile}") | ||
| if cafile: | ||
| args.append(f"--tls-ca-cert={cafile}") | ||
| tls_cert = Path(certfile) if certfile else None | ||
| tls_key = Path(keyfile) if keyfile else None | ||
| tls_ca_cert = Path(cafile) if cafile else None | ||
| tls_port = 0 if tls_key else None | ||
|
|
||
| if mode == "sync": | ||
| args.append("--interface=wsgi") | ||
| args.append("server:wsgi_app") | ||
| interface = "wsgi" | ||
| app = "server:wsgi_app" | ||
| else: | ||
| args.append("server:asgi_app") | ||
|
|
||
| proc = await asyncio.create_subprocess_exec( | ||
| "pyvoy", | ||
| *args, | ||
| stderr=asyncio.subprocess.STDOUT, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| env=_server_env(request), | ||
| ) | ||
| stdout = proc.stdout | ||
| assert stdout is not None | ||
| stdout = _tee_to_stderr(stdout) | ||
| interface = "asgi" | ||
| app = "server:asgi_app" | ||
|
|
||
| async def start(): | ||
| # Currently explicit env not accepted but it's safe to set it here. | ||
| os.environ["READ_MAX_BYTES"] = str(request.message_receive_limit) | ||
| async with PyvoyServer( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically switching to programmatic is a workaround for an issue that pyvoy only prints a single port even if there are several (for HTTP/3, when setting port=0, the TLS and QUIC port end up separate due to Envoy's behavior). But I figured it cleans things up anyways, it's still a subprocess, just not subprocess of subprocess.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is that something you'd want to fix upstream in pyvoy?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup intend to, though not to revert this back, I think programmatic works well here
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, looks reasonable to me. |
||
| app, | ||
| port=0, | ||
| tls_port=tls_port, | ||
| interface=interface, | ||
| tls_key=tls_key, | ||
| tls_cert=tls_cert, | ||
| tls_ca_cert=tls_ca_cert, | ||
| ) as server: | ||
| if ( | ||
| request.http_version == HTTPVersion.HTTP_VERSION_3 | ||
| and server.listener_port_quic | ||
| ): | ||
| port = server.listener_port_quic | ||
| else: | ||
| port = server.listener_port_tls or server.listener_port | ||
| port_future.set_result(port) | ||
| await asyncio.Future() # run until cancelled | ||
|
|
||
| task = asyncio.create_task(start()) | ||
| try: | ||
| async for line in stdout: | ||
| if b"listening on" in line: | ||
| port = int(line.strip().split(b"127.0.0.1:")[1]) | ||
| port_future.set_result(port) | ||
| break | ||
| await _consume_log(stdout) | ||
| await task | ||
| except asyncio.CancelledError: | ||
| proc.terminate() | ||
| await proc.wait() | ||
| task.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await task | ||
|
|
||
|
|
||
| async def serve_uvicorn( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just so we pipe through the environment that was already going to the subprocess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. While I thought it was a rare use case, I will add extra_env or something to
PyvoyServerto simplify this one