Skip to content

Seperate keepalive 0 from socket settimeout #876

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjavad
Copy link

@cjavad cjavad commented Dec 18, 2024

In certain conditions (MQTT over SSL, and also some websockets stuff) _keepalive is used as a socket timeout, which is all good until used with 0, which triggers as #42 describes it "an infinite loop" preventing any connection from being established. Originally in our testing only the SSL code path was triggered, and referring an older change (#578) _connect_timeout can be used as an alternative, which also semantically makes sense.

I noticed the same value was used on another socket for websocket transports, and with some quick testing both cases fail with keepalive 0:

Using test.mosquitto.org with websockets:

...
>>> client.connect(url, port, keepalive=0)
... (stack trace)
BlockingIOError: [Errno 11] Resource temporarily unavailable

And over SSL:

...
>>> client.connect(url, port, keepalive=0)
... (stack trace)
ssl.SSLWantReadError: The operation did not complete (read) (_ssl.c:1000)

Both TLS and WebSocket rely on initial communication beyond the socket connection to establish a connection which makes a timeout of 0 impossible to respect for either protocol.

Using a seperate value which is non-zero seems to make more sense, although the meaning of keepalive fits the settimeout functionality better, so perhaps only calling it for values greater than 0?

…epalive 0 to be set

Signed-off-by: Javad Asgari Shafique <[email protected]>
@Sporarum
Copy link

Sporarum commented May 2, 2025

What is the status on this PR ?

Merging it would help me a ton !

@Sporarum Sporarum mentioned this pull request May 2, 2025
@cjavad
Copy link
Author

cjavad commented May 2, 2025

Hi @Sporarum personally we ended up just using a keepalive value that was consistent with the rest of our system instead of disabling this feature, previously we maintained a runtime patch like this https://gist.github.com/cjavad/b804b4fada7ff0e18cf77d43b7f731dd, which can be adapted to also patch out websockets etc.

But i would imagine @'ing a maintainer could get this moving otherwise.

@Sporarum
Copy link

Sporarum commented May 2, 2025

In our case, we want a long keepalive, but a short connection timeout, which does not seem possible because of this bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants