search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Clarification regarding aleph.http/start-server's :idle-timeout option #609

Closed DerGuteMoritz closed 2 years ago

DerGuteMoritz commented 2 years ago

The docstring for aleph.http/start-server's :idle-timeout option states:

when set, forces keep-alive connections to be closed after an idle time, in milliseconds

I think this is too narrow: AFAICT the timeout also applies to non-keep-alive connections. Can you confirm this? If so, I would clarify the docstring accordingly.

arnaudgeiser commented 2 years ago

Yes, I confirm. We are using extensively :idle-timeout on non-persistent connections. We recently landed a fix regarding this : https://github.com/clj-commons/aleph/pull/601

KingMob commented 2 years ago

Yeah, we should update it. Non-persistent connections are pretty rare (HTTP/1.1 defaults to persistent connections, and HTTP/1.0 conns without keep-alive set are uncommon), but we're still applying the timeout duration to them, if set.

DerGuteMoritz commented 2 years ago

@arnaudgeiser Thanks! And also thanks for the fix - I think we ran into the same problem a few months ago but didn't get to the bottom of it, so we just ended up disabling keep-alive between the proxy and the backend server. Will re-evaluate now with 0.5.0 :pray:

@KingMob Good point re non-persistent connections being rare. But in fact, there is one situation where one is more likely to run into them, namely in the case of using nginx as a proxy server: It uses HTTP 1.0 by default and even when switching to HTTP 1.1, AFAIUI one has to explicitly enable it on the upstream.

That being said, I also find the current docstring a bit misleading: Because it mentions keep-alive explicitly, I assumed it referred to idle time between request/response exchanges on a persistent connection. In reality though, this timeout may fire at any point during such an exchange if no I/O operation happens for long enough: "Triggers an IdleStateEvent when a Channel has not performed read, write, or both operation for a while." I'll make a PR with an improvment suggestion!

arnaudgeiser commented 2 years ago

Yeah, we should update it. Non-persistent connections are pretty rare (HTTP/1.1 defaults to persistent connections, and HTTP/1.0 conns without keep-alive set are uncommon), but we're still applying the timeout duration to them, if set.

Sorry, I wasn't talking about persistent connections at the HTTP level but between two services.

I assumed it referred to idle time between request/response exchanges on a persistent connection. In reality though, this timeout may fire at any point during such an exchange if no I/O operation happens for long enough: "Triggers an IdleStateEvent when a Channel has not performed read, write, or both operation for a while." I'll make a PR with an improvment suggestion!

Exactly, the IdleStateHandler is all about channel activity/inactivity but not related to persistent/non-persistent connections at all.

KingMob commented 2 years ago

But in fact, there is one situation where one is more likely to run into them, namely in the case of using nginx as a proxy server: It uses HTTP 1.0 by default and even when switching to HTTP 1.1, AFAIUI one has to explicitly enable it on the upstream.

TIL!