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 no auto reconnect after close/connect in TCPclient #1298

Merged
merged 10 commits into from
Jan 27, 2023
Merged

fix no auto reconnect after close/connect in TCPclient #1298

merged 10 commits into from
Jan 27, 2023

Conversation

peufeu2
Copy link
Contributor

@peufeu2 peufeu2 commented Jan 26, 2023

Greetings,

AsyncModbusTcpClient.close() followed by connect() to reuse the object left it unable to auto-reconnect because delay_ms was set to 0 in close() but not reset to default setting in connect().

Added canceling of auto-reconnect background task in close().

Added check to avoid launching more than one auto-reconnect task in case user calls connect() too many times.

fixes #1198

pymodbus/client/tcp.py Outdated Show resolved Hide resolved
peufeu2 and others added 2 commits January 26, 2023 13:25
Co-authored-by: jan iversen <jancasacondor@gmail.com>
@janiversen
Copy link
Collaborator

Please also solve the CI errors.

@peufeu2
Copy link
Contributor Author

peufeu2 commented Jan 26, 2023

The stuff lint was complaining about should be OK
The other pytest failure comes from test_client.py:

    client.delay_ms = 5000
    await client.connect()

    run_coroutine(client._reconnect())  # pylint: disable=protected-access
    mock_sleep.assert_called_once_with(5)

connect() now overwrites delay_ms with the default in params (that solved the bug breaking reconnects after close()). So it overwrites the value set to "5000" and fails this test.

I'm not sure what to do here, because it's up to you to decide if client.delay_ms is an API that should be writable by the user or not. Although I think it makes little sense to have it writable because the reconnect functions in the client modify the value to increase the interval between reconnects, and it is reset to the default value from params after a successful connection. So if the user writes something to it, it'll be short lived anyway...

@janiversen
Copy link
Collaborator

janiversen commented Jan 27, 2023

delay_ms is not part of the API, but in many of the tests are allowed to use internal values.

Please correct the tests, maybe the tests should simply use the timeout parameter.

Please remember some of the tests are very old, so changing them is quite ok even removing some might be ok. When doing the final review, I will be extra careful when looking at the test changes.

@peufeu2
Copy link
Contributor Author

peufeu2 commented Jan 27, 2023

Done, looks good!

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
Copy link
Collaborator

Seems there are a problem with python 3.11, I have restarted the job to see if it is a real error.

@janiversen janiversen merged commit 97ffffc into pymodbus-dev:dev Jan 27, 2023
alexrudd2 pushed a commit to alexrudd2/pymodbus that referenced this pull request Feb 21, 2023
…1298)

fix no auto reconnect after close/connect in TCPclient
Co-authored-by: jan iversen <jancasacondor@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 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.

calling client.close() does not terminate the client process but causes a reconnect.
2 participants