Skip to content

Seperate keepalive 0 from socket settimeout #876

Open
cjavad wants to merge 1 commit into
eclipse-paho:masterfrom
cjavad:fix_keepalive_0_ssl
Open

Seperate keepalive 0 from socket settimeout #876
cjavad wants to merge 1 commit into
eclipse-paho:masterfrom
cjavad:fix_keepalive_0_ssl

Conversation

@cjavad

@cjavad cjavad commented Dec 18, 2024

Copy link
Copy Markdown

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 <me@javad.sh>
@Sporarum

Sporarum commented May 2, 2025

Copy link
Copy Markdown

What is the status on this PR ?

Merging it would help me a ton !

@Sporarum Sporarum mentioned this pull request May 2, 2025
@cjavad

cjavad commented May 2, 2025

Copy link
Copy Markdown
Author

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

Sporarum commented May 2, 2025

Copy link
Copy Markdown

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