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

Fix bug report request IDs being reset when using multiple KoreClients #4480

Merged
merged 18 commits into from
Jun 27, 2024
Merged
Changes from 9 commits
Commits
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
92 changes: 56 additions & 36 deletions pyk/src/pyk/kore/rpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,42 @@ def __init__(self, message: str, code: int, data: Any = None):


class Transport(ContextManager['Transport'], ABC):
_bug_report: BugReport | None
_bug_report_id: str | None

def __init__(self, bug_report_id: str | None = None, bug_report: BugReport | None = None) -> None:
self._bug_report_id = bug_report_id
self._bug_report = bug_report
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, bug_report_id: str | None = None, bug_report: BugReport | None = None) -> None:
self._bug_report_id = bug_report_id
self._bug_report = bug_report
def __init__(self, bug_report: BugReport | None = None, bug_report_id: str | None = None) -> None:
self._bug_report = bug_report
self._bug_report_id = bug_report_id

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valid to have a bug_report_id but not a bug_report? If it isn't, ValueError should be raised when it happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this error.


def request_with_bug_report(self, req: str, request_id: int) -> str:
base_name = self._bug_report_id if self._bug_report_id is not None else 'kore_rpc'
req_name = f'{base_name}/{self._client_id}/{request_id:03}'
if self._bug_report:
bug_report_request = f'{req_name}_request.json'
self._bug_report.add_file_contents(req, Path(bug_report_request))
self._bug_report.add_command(self.command(req_name, bug_report_request))

server_addr = self.description()
_LOGGER.debug(f'Sending request to {server_addr}: {req}')
resp = self.request(req)
if not resp:
raise RuntimeError('Empty response received')
_LOGGER.debug(f'Received response from {server_addr}: {resp}')

if self._bug_report:
bug_report_response = f'{req_name}_response.json'
self._bug_report.add_file_contents(resp, Path(bug_report_response))
self._bug_report.add_command(
[
'diff',
'-b',
'-s',
f'{req_name}_actual.json',
f'{req_name}_response.json',
]
)
return resp

@abstractmethod
def request(self, req: str) -> str: ...
tothtamas28 marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -69,11 +105,15 @@ def __exit__(self, *args: Any) -> None:
def close(self) -> None: ...

@abstractmethod
def command(self, bug_report_id: str, old_id: int, bug_report_request: str) -> list[str]: ...
def command(self, req_name: str, bug_report_request: str) -> list[str]: ...
tothtamas28 marked this conversation as resolved.
Show resolved Hide resolved

@abstractmethod
def description(self) -> str: ...

@property
def _client_id(self) -> str:
tothtamas28 marked this conversation as resolved.
Show resolved Hide resolved
return str(id(self))


class TransportType(Enum):
SINGLE_SOCKET = auto()
Expand All @@ -87,7 +127,8 @@ class SingleSocketTransport(Transport):
_sock: socket.socket
_file: TextIO

def __init__(self, host: str, port: int, *, timeout: int | None = None):
def __init__(self, host: str, port: int, *, timeout: int | None = None, **kwargs: Any):
super().__init__(**kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider just explicitly listing kwargs here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

self._host = host
self._port = port
self._sock = self._create_connection(host, port, timeout)
Expand Down Expand Up @@ -117,7 +158,7 @@ def close(self) -> None:
self._file.close()
self._sock.close()

def command(self, bug_report_id: str, old_id: int, bug_report_request: str) -> list[str]:
def command(self, req_name: str, bug_report_request: str) -> list[str]:
return [
'cat',
bug_report_request,
Expand All @@ -127,7 +168,7 @@ def command(self, bug_report_id: str, old_id: int, bug_report_request: str) -> l
self._host,
str(self._port),
'>',
f'rpc_{bug_report_id}/{old_id:03}_actual.json',
f'{req_name}_actual.json',
]

def request(self, req: str) -> str:
Expand All @@ -146,15 +187,16 @@ class HttpTransport(Transport):
_port: int
_timeout: int | None

def __init__(self, host: str, port: int, *, timeout: int | None = None):
def __init__(self, host: str, port: int, *, timeout: int | None = None, **kwargs: Any):
super().__init__(**kwargs)
self._host = host
self._port = port
self._timeout = timeout

def close(self) -> None:
pass

def command(self, bug_report_id: str, old_id: int, bug_report_request: str) -> list[str]:
def command(self, req_name: str, bug_report_request: str) -> list[str]:
return [
'curl',
'-X',
Expand All @@ -165,7 +207,7 @@ def command(self, bug_report_id: str, old_id: int, bug_report_request: str) -> l
'@' + bug_report_request,
'http://' + self._host + ':' + str(self._port),
'>',
f'rpc_{bug_report_id}/{old_id:03}_actual.json',
f'{req_name}_actual.json',
]

def request(self, req: str) -> str:
Expand Down Expand Up @@ -258,8 +300,6 @@ class JsonRpcClient(ContextManager['JsonRpcClient']):

_transport: Transport
_req_id: int
_bug_report: BugReport | None
_bug_report_id: str

def __init__(
self,
Expand All @@ -272,14 +312,16 @@ def __init__(
transport: TransportType = TransportType.SINGLE_SOCKET,
):
if transport is TransportType.SINGLE_SOCKET:
self._transport = SingleSocketTransport(host, port, timeout=timeout)
self._transport = SingleSocketTransport(
host, port, timeout=timeout, bug_report=bug_report, bug_report_id=bug_report_id
)
elif transport is TransportType.HTTP:
self._transport = HttpTransport(host, port, timeout=timeout)
self._transport = HttpTransport(
host, port, timeout=timeout, bug_report=bug_report, bug_report_id=bug_report_id
)
else:
raise AssertionError()
self._req_id = 1
self._bug_report = bug_report
self._bug_report_id = bug_report_id if bug_report_id is not None else str(id(self))

def __enter__(self) -> JsonRpcClient:
return self
Expand All @@ -304,29 +346,7 @@ def request(self, method: str, **params: Any) -> dict[str, Any]:
server_addr = self._transport.description()
_LOGGER.info(f'Sending request to {server_addr}: {old_id} - {method}')
tothtamas28 marked this conversation as resolved.
Show resolved Hide resolved
req = json.dumps(payload)
if self._bug_report:
bug_report_request = f'rpc_{self._bug_report_id}/{old_id:03}_request.json'
self._bug_report.add_file_contents(req, Path(bug_report_request))
self._bug_report.add_command(self._transport.command(self._bug_report_id, old_id, bug_report_request))

_LOGGER.debug(f'Sending request to {server_addr}: {req}')
resp = self._transport.request(req)
if not resp:
raise RuntimeError('Empty response received')
_LOGGER.debug(f'Received response from {server_addr}: {resp}')

if self._bug_report:
bug_report_response = f'rpc_{self._bug_report_id}/{old_id:03}_response.json'
self._bug_report.add_file_contents(resp, Path(bug_report_response))
self._bug_report.add_command(
[
'diff',
'-b',
'-s',
f'rpc_{self._bug_report_id}/{old_id:03}_actual.json',
f'rpc_{self._bug_report_id}/{old_id:03}_response.json',
]
)
resp = self._transport.request_with_bug_report(req, old_id)

data = json.loads(resp)
self._check(data)
Expand Down
Loading