-
Notifications
You must be signed in to change notification settings - Fork 161
Streaming support for JsonRpcServer #4879
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
base: develop
Are you sure you want to change the base?
Changes from all commits
b474eee
c91c73f
9a302d9
9ae8814
ef20f25
de82e95
d4836ef
298cc40
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 |
|---|---|---|
|
|
@@ -3,10 +3,11 @@ | |
| import json | ||
| import logging | ||
| from abc import ABC, abstractmethod | ||
| from collections.abc import Iterator | ||
| from dataclasses import dataclass | ||
| from functools import partial | ||
| from http.server import BaseHTTPRequestHandler, HTTPServer | ||
| from typing import TYPE_CHECKING, Any, Final, NamedTuple | ||
| from typing import TYPE_CHECKING, NamedTuple | ||
|
|
||
| from typing_extensions import Protocol | ||
|
|
||
|
|
@@ -15,6 +16,7 @@ | |
| if TYPE_CHECKING: | ||
| from collections.abc import Callable | ||
| from pathlib import Path | ||
| from typing import Any, Final | ||
|
|
||
|
|
||
| _LOGGER: Final = logging.getLogger(__name__) | ||
|
|
@@ -86,7 +88,7 @@ class JsonRpcBatchRequest(NamedTuple): | |
| class JsonRpcResult(ABC): | ||
|
|
||
| @abstractmethod | ||
| def encode(self) -> bytes: ... | ||
| def encode(self) -> Iterator[bytes]: ... | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
|
|
@@ -96,7 +98,7 @@ class JsonRpcError(JsonRpcResult): | |
| message: str | ||
| id: str | int | None | ||
|
|
||
| def to_json(self) -> dict[str, Any]: | ||
| def wrap_response(self) -> dict[str, Any]: | ||
| return { | ||
| 'jsonrpc': JsonRpcServer.JSONRPC_VERSION, | ||
| 'error': { | ||
|
|
@@ -106,32 +108,40 @@ def to_json(self) -> dict[str, Any]: | |
| 'id': self.id, | ||
| } | ||
|
|
||
| def encode(self) -> bytes: | ||
| return json.dumps(self.to_json()).encode('ascii') | ||
| def encode(self) -> Iterator[bytes]: | ||
| yield json.dumps(self.wrap_response()).encode('utf-8') | ||
|
Contributor
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. This yields the whole data = json.dumps(self.wrap_response()).encode('utf-8')
yield from (bytes([b]) for b in data)
Member
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. Don't think we need this fine granularity. The purpose of splitting the output stream into chunks is to keep the memory usage low. For example, |
||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class JsonRpcSuccess(JsonRpcResult): | ||
| payload: Any | ||
| id: Any | ||
|
|
||
| def to_json(self) -> dict[str, Any]: | ||
| return { | ||
| 'jsonrpc': JsonRpcServer.JSONRPC_VERSION, | ||
| 'result': self.payload, | ||
| 'id': self.id, | ||
| } | ||
|
|
||
| def encode(self) -> bytes: | ||
| return json.dumps(self.to_json()).encode('ascii') | ||
| def encode(self) -> Iterator[bytes]: | ||
| id_encoded = json.dumps(self.id) | ||
| version_encoded = json.dumps(JsonRpcServer.JSONRPC_VERSION) | ||
| yield f'{{"jsonrpc": {version_encoded}, "id": {id_encoded}, "result": '.encode() | ||
| if isinstance(self.payload, Iterator): | ||
| yield from self.payload | ||
|
Contributor
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
Member
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. Not necessarily. |
||
| else: | ||
| yield json.dumps(self.payload).encode('utf-8') | ||
|
Contributor
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. This also yields the whole |
||
| yield b'}' | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class JsonRpcBatchResult(JsonRpcResult): | ||
| results: tuple[JsonRpcError | JsonRpcSuccess, ...] | ||
|
|
||
| def encode(self) -> bytes: | ||
| return json.dumps([result.to_json() for result in self.results]).encode('ascii') | ||
| def encode(self) -> Iterator[bytes]: | ||
| yield b'[' | ||
| first = True | ||
| for result in self.results: | ||
| if not first: | ||
| yield b',' | ||
| else: | ||
| first = False | ||
| yield from result.encode() | ||
| yield b']' | ||
|
|
||
|
|
||
| class JsonRpcRequestHandler(BaseHTTPRequestHandler): | ||
|
|
@@ -143,8 +153,10 @@ def __init__(self, methods: dict[str, JsonRpcMethod], *args: Any, **kwargs: Any) | |
|
|
||
| def _send_response(self, response: JsonRpcResult) -> None: | ||
| self.send_response_headers() | ||
| response_bytes = response.encode() | ||
| self.wfile.write(response_bytes) | ||
| response_body = response.encode() | ||
| for chunk in response_body: | ||
| self.wfile.write(chunk) | ||
| self.wfile.flush() | ||
|
|
||
| def send_response_headers(self) -> None: | ||
| self.send_response(200) | ||
|
|
||
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.
The PR description states
but it introduces what look like breaking changes. Are there other clients to this library other than
kontrol-node?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.
Good catch. I cannot think of a use case where a client library actually uses any
JsonRpcResultsubclasses directly. They were introduced as an internal abstraction layer when I implemented batch requests. Also, I'm not aware of any other user thankontrol-node. Therefore, the risk is very low here and can be easily addressed in downstream repositories.