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.

Sign up for GitHub

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

Jump to bottom

Only log when TransportLimits actually changed #1183

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Only log when TransportLimits actually changed #1183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c28d890
Add unit tests for TransportLimits
rnestler Jan 16, 2023
4de28be
Only log message when limits change
rnestler Jan 16, 2023
00bf33a
Move creation of new_limits into helper
rnestler Jan 18, 2023
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
30 changes: 20 additions & 10 deletions asyncua/common/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from datetime import datetime, timedelta
import logging
import copy
from typing_extensions import Literal

from asyncua import ua
from asyncua.ua.uaerrors import UaInvalidParameterError
Expand Down Expand Up @@ -51,17 +52,30 @@ def is_chunk_count_within_limit(self, sz: int) -> bool:
_logger.error("Number of message chunks: %s is > configured max chunk count: %s", sz, self.max_chunk_count)
return within_limit

def _set_new_limits_from_ack(self, ack: ua.Acknowledge, role: Literal["client", "server"]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

if you really need that Role, it should be an enum.
But in that case I would jsut create two methods, one for server and one for client. That is simpler

max_recv_buffer = ack.ReceiveBufferSize if role == "client" else ack.SendBufferSize
max_send_buffer = ack.SendBufferSize if role == "client" else ack.ReceiveBufferSize

new_limits = TransportLimits(
max_chunk_count=ack.MaxChunkCount,
max_recv_buffer=max_recv_buffer,
max_send_buffer=max_send_buffer,
max_message_size=ack.MaxMessageSize,
)
Comment on lines +56 to +64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm now not sure if it makes sense to create the new_limits here or if we should keep this in the other methods, since it differs between client and server.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry That MR disapeard into the backlog.. but yes inline would be better I think

if new_limits != self:
self.max_chunk_count = new_limits.max_chunk_count
self.max_recv_buffer = new_limits.max_recv_buffer
self.max_send_buffer = new_limits.max_send_buffer
self.max_message_size = new_limits.max_message_size
_logger.info("updating %s limits to: %s", role, self)

def create_acknowledge_and_set_limits(self, msg: ua.Hello) -> ua.Acknowledge:
ack = ua.Acknowledge()
ack.ReceiveBufferSize = min(msg.ReceiveBufferSize, self.max_send_buffer)
ack.SendBufferSize = min(msg.SendBufferSize, self.max_recv_buffer)
ack.MaxChunkCount = self._select_limit(msg.MaxChunkCount, self.max_chunk_count)
ack.MaxMessageSize = self._select_limit(msg.MaxMessageSize, self.max_message_size)
self.max_chunk_count = ack.MaxChunkCount
self.max_recv_buffer = ack.SendBufferSize
self.max_send_buffer = ack.ReceiveBufferSize
self.max_message_size = ack.MaxMessageSize
_logger.info("updating server limits to: %s", self)
self._set_new_limits_from_ack(ack, "server")
return ack

def create_hello_limits(self, msg: ua.Hello) -> ua.Hello:
Expand All @@ -72,11 +86,7 @@ def create_hello_limits(self, msg: ua.Hello) -> ua.Hello:
return msg

def update_client_limits(self, msg: ua.Acknowledge) -> None:
self.max_chunk_count = msg.MaxChunkCount
self.max_recv_buffer = msg.ReceiveBufferSize
self.max_send_buffer = msg.SendBufferSize
self.max_message_size = msg.MaxMessageSize
_logger.info("updating client limits to: %s", self)
self._set_new_limits_from_ack(msg, "client")


class MessageChunk:
Expand Down
22 changes: 22 additions & 0 deletions tests/common/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import pytest

from asyncua.common.connection import TransportLimits, ua

@pytest.mark.parametrize('transport_limits, ua_hello, expected_ack, expected_transport_limits', [
(TransportLimits(), ua.Hello(), ua.Acknowledge(0, 65535, 65535, 104857600, 1601), TransportLimits()),
(TransportLimits(), ua.Hello(0, 100, 200, 1000, 10), ua.Acknowledge(0, 100, 200, 1000, 10), TransportLimits(200, 100, 10, 1000)),
])
def test_create_acknowledge_and_set_limits(transport_limits, ua_hello, expected_ack, expected_transport_limits):
ua_ack = transport_limits.create_acknowledge_and_set_limits(ua_hello)
assert ua_ack == expected_ack
assert transport_limits == expected_transport_limits


@pytest.mark.parametrize('ua_ack, expected_transport_limits', [
(ua.Acknowledge(0, 65535, 65535, 104857600, 1601), TransportLimits()),
(ua.Acknowledge(0, 100, 200, 1000, 10), TransportLimits(100, 200, 10, 1000)),
])
def test_update_client_limits(ua_ack, expected_transport_limits):
transport_limits = TransportLimits()
transport_limits.update_client_limits(ua_ack)
assert transport_limits == expected_transport_limits
Loading