search

jmccrohan / pysolarmanv5

A python module to interact with Solarman Data Logging Sticks
MIT License
116 stars 25 forks source link

Fix socket closing, episode 2 #53

Closed sofkaski closed 2 months ago

sofkaski commented 2 months ago

While making changes to address linter findings I found out that _data_receiver will re-open a connection that is closed by disconnect call if auto-reconnect is on.

Added disabling of auto-reconnect when the connection is closed by the api user by calling disconnect.

(Linter fixes PR will come on top of this)

sofkaski commented 2 months ago

(Äh... forgot to rebase before making the PR... fixed)

githubDante commented 2 months ago

I found out that _data_receiver will re-open a connection that is closed by disconnect call if auto-reconnect is on.

No, it will not. The thread will exit silently at most 500ms (the POLL timeout) after the call to disconnect().

https://github.com/jmccrohan/pysolarmanv5/blob/4c9b206d46580d8502957553e424602d5e42c2aa/pysolarmanv5/pysolarmanv5.py#L297-L299

Since the socket setup method is not publicly exposed, however, setting the flag to False probably will not be a problem.

Edit: Small test update for confirmation - https://github.com/githubDante/pysolarmanv5/commit/637086987357f4256b4de60259b0a30392252738

sofkaski commented 2 months ago

No, it will not. The thread will exit silently at most 500ms (the POLL timeout) after the call to disconnect().

Ah.. you right @githubDante. My observation was from a version where I moved self._reader_exit.is_set() to used as the loop control in an attempt to kill one branch from the loop. But then I abandoned that later.

I propose to close this PR without merging. Altering a parameter that has been set via the API is not a good practice anyways.