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

fixes access to asyncio loop via loop property of SerialTransport #1804

Merged
merged 8 commits into from
Oct 8, 2023

Conversation

MAKOMO
Copy link
Contributor

@MAKOMO MAKOMO commented Oct 8, 2023

The property loop of SerialTransport (transport_seria.py) is defined as

    @property
    def loop(self):
        """Return asyncio event loop."""
        return self._protocol.loop

with self._protocol of type asyncio.protocols.BaseProtocol which does not feature an attribute loop, but instead holds the asyncio event loop in the attribute _loop thus this definition should read correctly

    @property
    def loop(self):
        """Return asyncio event loop."""
        return self._protocol._loop

There are two further occurrences of accessing the event loop of the _protocol instances via the attribute loop instead of _loop (in _read_ready and _write_ready). This PR fixes those and adds a test that demonstrates the issue.

@janiversen
Copy link
Collaborator

Please solve the CI errors.

MAKOMO added 5 commits October 8, 2023 12:19
… but there seems no method to get the loop from a asyncio.BaseProtocol.
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

The production code change is ok, but please remove the test, for the reasons mentioned.

@@ -292,6 +292,17 @@ async def test_init(self):
"""Test null modem init"""
SerialTransport(asyncio.get_running_loop(), mock.Mock(), "dummy")

async def test_loop(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not test that the libraries work correctly, so I see this test as not needed.

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 added this test more as illustration that something is wrong in the original code. I will remove it.

Please check again the production code change. Is the "protocol" event loop necessarily always the same as the one of the transport? Anyhow, the original access was faulty.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

LGtM, thanks.

@janiversen janiversen merged commit 7e3e9d9 into pymodbus-dev:dev Oct 8, 2023
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants