Re: [PATCH v2] Add a receive_timeout option for sockets
On Wed, Nov 30, 2022 at 08:03:13AM +0100, Petr Pisar wrote: V Tue, Nov 29, 2022 at 11:28:29AM -0800, Kevin J. McCarthy napsal(a): If that's not always the case then we can add a SNDTIMEO too. Also an option. Though, I cannot see a reason why somebody wanted a different timeout for reading and for writing. Adding SNDTIMEO later would require adding a new configuration option. I wanted to prevent from proliferating them. I'm fine with adding configuration options if they make sense. If it's generally agreed that _no one_ would want to have a different RCVTIMEO and SNDTIMEO, then I'm also fine with just $socket_timeout. But for low level socket timeout tweaking, I'm not convinced someone eventually won't come and ask for separate values, for some reason or another. The $imap_poll_timeout code works in that way, at least, sending a NOOP and polling those seconds for a response. Have you considered another network-accessed mail boxes, like POP3? There is no general application-level timeout for them in Mutt. imap_poll_timeout is only for IMAP, isn't? Also I think that imap_poll_timeout applies after read()/write() returns. If one of them blocks, imap_poll_timeout won't help. I think this is getting off-topic for this thread, unless I'm misunderstanding. The IMAP timeout code does a write() (usually just "NOOP") followed by a poll(). As you say it's not bullet-proof but does help in a lot of cases, combined with the auto-reconnect code. -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA signature.asc Description: PGP signature
Re: [PATCH v2] Add a receive_timeout option for sockets
V Tue, Nov 29, 2022 at 11:28:29AM -0800, Kevin J. McCarthy napsal(a): > On Tue, Nov 29, 2022 at 09:44:25AM +0100, Petr Pisar wrote: > >If a network is unreliable, you will have similar problem with writing > >to the TCP socket. I think it would be better to rename the option to > >socket_timeout and use the same value for both setsockopt(, SO_RCVTIMEO, ) > >and > >setsockopt(, SO_SNDTIMEO, ). > > Thanks for your feedback Petr. In my experience, I've found that > writing (small amounts) to a "hung" socket doesn't seem to be a problem > - the computer buffers it or something; it's trying to read the response > back that hangs. Yes, operating system usually buffers written data, until a TCP window size fills in. Then the write starts blocking. However, POSIX states that write() and sendto() are in general a blocking operation. > The $imap_poll_timeout code works in that way, at > least, sending a NOOP and polling those seconds for a response. > Have you considered another network-accessed mail boxes, like POP3? There is no general application-level timeout for them in Mutt. imap_poll_timeout is only for IMAP, isn't? Also I think that imap_poll_timeout applies after read()/write() returns. If one of them blocks, imap_poll_timeout won't help. For you reference, Mutt has now: connect_timeout imap_poll_timeout pgp_timeout smime_timeout timeout > If that's not always the case then we can add a SNDTIMEO too. > Also an option. Though, I cannot see a reason why somebody wanted a different timeout for reading and for writing. Adding SNDTIMEO later would require adding a new configuration option. I wanted to prevent from proliferating them. -- Petr signature.asc Description: PGP signature
Re: [PATCH v2] Add a receive_timeout option for sockets
On Tue, Nov 29, 2022 at 11:36:59AM -0800, Matthew Sotoudeh via Mutt-dev wrote: Thanks for pointing that out ! To second Kevin's comments below, I personally didn't find a SNDTIMEO necessary. Also worth noting that, at least in my case, large writes (sending email, etc.) happen while I'm sitting down, so a much more stable network connection vs., e.g., background downloading of new mail. Happy to add a SO_SNDTIMEO while we're at it just in case/for consistency, though, if people prefer that. Since setsockopt() itself separates the timeout types, I wouldn't be opposed to adding both a $socket_receive_timeout and $socket_send_timeout config setting. That way, if for some reason a person wanted or needed to tweak them separately, they can. These are definitely "advanced tweaking" options, so I think it's okay to separate them. But if we add both I like having a common "socket_" prefix so they are grouped in the manual, and it's immediately clear they are referring to socket settings. -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA signature.asc Description: PGP signature
Re: [PATCH v2] Add a receive_timeout option for sockets
On Tue, Nov 29, 2022 at 11:35:11AM -0800, Kevin J. McCarthy wrote: > On Tue, Nov 29, 2022 at 02:19:26PM +0100, Oswald Buddenhagen wrote: > > On Mon, Nov 28, 2022 at 11:24:13PM -0800, Matthew Sotoudeh via Mutt-dev > > wrote: > > > + ** Causes Mutt to timeout any socket read operation (e.g. SSL_read) > > > after > > > + ** this many seconds. A zero (default) or negative value causes Mutt > > > to wait > > > + ** indefinitely for the read to complete. > > > > > have you checked how this plays along with imap IDLE? > > Thanks Oswald. I'll take a closer look before committing, but I don't think > it should have a particular effect, at least for Mutt's implementation. > Mutt sends IDLE, polls the socket for up to $imap_poll_timeout, and then > reads looking for the "+" response. It's possible I'm not exercising all corner cases here, but to add some empirical data: just tried running this patch with imap_idle and a relatively small receive_timeout for a bit, and didn't notice any difference in behavior. Even with receive_timeout=0, imap_idle=yes, my Mutt config only seems to (intentionally) do long-blocking reads on stdin/waiting for user input. imap_check_mailbox sets up the idle with the server and returns quickly after that, with or without receive_timeout. Thanks for all the feedback, Matthew
Re: [PATCH v2] Add a receive_timeout option for sockets
On Mon, Nov 28, 2022 at 11:24:13PM -0800, Matthew Sotoudeh via Mutt-dev wrote:
> On an unreliable connection (e.g., laptop put to sleep and changing wifi
> networks) I've had mutt fairly regularly become stuck in SSL_read and
> have to be killed.
>
> Per some of the comments on
> https://stackoverflow.com/questions/46517875/ssl-read-blocks-indefinitely
> adding a timeout to the socket should carry over to the SSL_read call.
>
> Using this receive_timeout option appears to resolve the issue for me,
> thought others may find it useful.
>
> Signed-off-by: Matthew Sotoudeh
Thanks, Matthew. Looks good other than 1 suggestion below.
Acked-by: David Vernet
> ---
>
> Thanks for the great tips! Hope this is a bit nicer formatted :)
> (Also, happy to move to gitlab if preferable & more changes desired for
> this. Sent here just bc doc/devel-notes.txt suggested.)
>
> Changelog per recent David/Kevin suggestions:
> * move setsockopt to right before the connect
> * check return value of setsockopt, log the error if it fails
>
> globals.h | 1 +
> init.h| 7 +++
> mutt_socket.c | 8
> 3 files changed, 16 insertions(+)
>
> diff --git a/globals.h b/globals.h
> index 06ce410e..b782114e 100644
> --- a/globals.h
> +++ b/globals.h
> @@ -236,6 +236,7 @@ WHERE short PagerContext;
> WHERE short PagerIndexLines;
> WHERE short PagerSkipQuotedContext;
> WHERE short ReadInc;
> +WHERE short ReceiveTimeout;
> WHERE short ReflowWrap;
> WHERE short SaveHist;
> WHERE short SendmailWait;
> diff --git a/init.h b/init.h
> index e160d3d3..e268d796 100644
> --- a/init.h
> +++ b/init.h
> @@ -3221,6 +3221,13 @@ struct option_t MuttVars[] = {
>** .pp
>** Also see $$postponed variable.
>*/
> + { "receive_timeout", DT_NUM, R_NONE, {.p=}, {.l=0} },
> + /*
> + ** .pp
> + ** Causes Mutt to timeout any socket read operation (e.g. SSL_read) after
> + ** this many seconds. A zero (default) or negative value causes Mutt to
> wait
> + ** indefinitely for the read to complete.
> + */
>{ "record",DT_PATH, R_NONE, {.p=}, {.p="~/sent"} },
>/*
>** .pp
> diff --git a/mutt_socket.c b/mutt_socket.c
> index 3e192072..9468319e 100644
> --- a/mutt_socket.c
> +++ b/mutt_socket.c
> @@ -433,6 +433,14 @@ static int socket_connect (int fd, struct sockaddr* sa)
>
>save_errno = 0;
>
> + if (ReceiveTimeout > 0)
> + {
> +struct timeval tv = { ReceiveTimeout, 0 };
> +if (setsockopt (fd, SOL_SOCKET, SO_RCVTIMEO, (char *), sizeof(tv)) <
> 0)
I personally don't think the < 0 is necessary here. setsockopt() fails
on any non-zero return value. connect() checks < 0 though, so it's
probably fine to leave it as is to match the rest of the code.
> + dprint (1, (debugfile, "socket_connect: error setting receive timeout
> (%s)\n",
> + strerror(errno)));
> + }
> +
>if (connect (fd, sa, sa_size) < 0)
>{
> save_errno = errno;
> --
> 2.34.1
>
Re: [PATCH v2] Add a receive_timeout option for sockets
Thanks for pointing that out ! To second Kevin's comments below, I personally didn't find a SNDTIMEO necessary. Also worth noting that, at least in my case, large writes (sending email, etc.) happen while I'm sitting down, so a much more stable network connection vs., e.g., background downloading of new mail. Happy to add a SO_SNDTIMEO while we're at it just in case/for consistency, though, if people prefer that. Best, Matthew On Tue, Nov 29, 2022 at 11:28:29AM -0800, Kevin J. McCarthy wrote: > On Tue, Nov 29, 2022 at 09:44:25AM +0100, Petr Pisar wrote: > > If a network is unreliable, you will have similar problem with writing > > to the TCP socket. I think it would be better to rename the option to > > socket_timeout and use the same value for both setsockopt(, SO_RCVTIMEO, ) > > and > > setsockopt(, SO_SNDTIMEO, ). > > Thanks for your feedback Petr. In my experience, I've found that writing > (small amounts) to a "hung" socket doesn't seem to be a problem - the > computer buffers it or something; it's trying to read the response back that > hangs. The $imap_poll_timeout code works in that way, at least, sending a > NOOP and polling those seconds for a response. > > If that's not always the case then we can add a SNDTIMEO too. > > -- > Kevin J. McCarthy > GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
Re: [PATCH v2] Add a receive_timeout option for sockets
On Tue, Nov 29, 2022 at 02:19:26PM +0100, Oswald Buddenhagen wrote: On Mon, Nov 28, 2022 at 11:24:13PM -0800, Matthew Sotoudeh via Mutt-dev wrote: + ** Causes Mutt to timeout any socket read operation (e.g. SSL_read) after + ** this many seconds. A zero (default) or negative value causes Mutt to wait + ** indefinitely for the read to complete. have you checked how this plays along with imap IDLE? Thanks Oswald. I'll take a closer look before committing, but I don't think it should have a particular effect, at least for Mutt's implementation. Mutt sends IDLE, polls the socket for up to $imap_poll_timeout, and then reads looking for the "+" response. -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA signature.asc Description: PGP signature
Re: [PATCH v2] Add a receive_timeout option for sockets
On Tue, Nov 29, 2022 at 09:44:25AM +0100, Petr Pisar wrote: If a network is unreliable, you will have similar problem with writing to the TCP socket. I think it would be better to rename the option to socket_timeout and use the same value for both setsockopt(, SO_RCVTIMEO, ) and setsockopt(, SO_SNDTIMEO, ). Thanks for your feedback Petr. In my experience, I've found that writing (small amounts) to a "hung" socket doesn't seem to be a problem - the computer buffers it or something; it's trying to read the response back that hangs. The $imap_poll_timeout code works in that way, at least, sending a NOOP and polling those seconds for a response. If that's not always the case then we can add a SNDTIMEO too. -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA signature.asc Description: PGP signature
Re: [PATCH v2] Add a receive_timeout option for sockets
On Mon, Nov 28, 2022 at 11:24:13PM -0800, Matthew Sotoudeh via Mutt-dev wrote: + ** Causes Mutt to timeout any socket read operation (e.g. SSL_read) after + ** this many seconds. A zero (default) or negative value causes Mutt to wait + ** indefinitely for the read to complete. have you checked how this plays along with imap IDLE?
Re: [PATCH v2] Add a receive_timeout option for sockets
V Mon, Nov 28, 2022 at 11:24:13PM -0800, Matthew Sotoudeh via Mutt-dev
napsal(a):
> On an unreliable connection (e.g., laptop put to sleep and changing wifi
> networks) I've had mutt fairly regularly become stuck in SSL_read and
> have to be killed.
>
> Per some of the comments on
> https://stackoverflow.com/questions/46517875/ssl-read-blocks-indefinitely
> adding a timeout to the socket should carry over to the SSL_read call.
>
> Using this receive_timeout option appears to resolve the issue for me,
> thought others may find it useful.
>
[...]
> + { "receive_timeout", DT_NUM, R_NONE, {.p=}, {.l=0} },
[...]
> + if (ReceiveTimeout > 0)
> + {
> +struct timeval tv = { ReceiveTimeout, 0 };
> +if (setsockopt (fd, SOL_SOCKET, SO_RCVTIMEO, (char *), sizeof(tv)) <
> 0)
> + dprint (1, (debugfile, "socket_connect: error setting receive timeout
> (%s)\n",
> + strerror(errno)));
> + }
> +
If a network is unreliable, you will have similar problem with writing
to the TCP socket. I think it would be better to rename the option to
socket_timeout and use the same value for both setsockopt(, SO_RCVTIMEO, ) and
setsockopt(, SO_SNDTIMEO, ).
-- Petr
signature.asc
Description: PGP signature
[PATCH v2] Add a receive_timeout option for sockets
On an unreliable connection (e.g., laptop put to sleep and changing wifi
networks) I've had mutt fairly regularly become stuck in SSL_read and
have to be killed.
Per some of the comments on
https://stackoverflow.com/questions/46517875/ssl-read-blocks-indefinitely
adding a timeout to the socket should carry over to the SSL_read call.
Using this receive_timeout option appears to resolve the issue for me,
thought others may find it useful.
Signed-off-by: Matthew Sotoudeh
---
Thanks for the great tips! Hope this is a bit nicer formatted :)
(Also, happy to move to gitlab if preferable & more changes desired for
this. Sent here just bc doc/devel-notes.txt suggested.)
Changelog per recent David/Kevin suggestions:
* move setsockopt to right before the connect
* check return value of setsockopt, log the error if it fails
globals.h | 1 +
init.h| 7 +++
mutt_socket.c | 8
3 files changed, 16 insertions(+)
diff --git a/globals.h b/globals.h
index 06ce410e..b782114e 100644
--- a/globals.h
+++ b/globals.h
@@ -236,6 +236,7 @@ WHERE short PagerContext;
WHERE short PagerIndexLines;
WHERE short PagerSkipQuotedContext;
WHERE short ReadInc;
+WHERE short ReceiveTimeout;
WHERE short ReflowWrap;
WHERE short SaveHist;
WHERE short SendmailWait;
diff --git a/init.h b/init.h
index e160d3d3..e268d796 100644
--- a/init.h
+++ b/init.h
@@ -3221,6 +3221,13 @@ struct option_t MuttVars[] = {
** .pp
** Also see $$postponed variable.
*/
+ { "receive_timeout", DT_NUM, R_NONE, {.p=}, {.l=0} },
+ /*
+ ** .pp
+ ** Causes Mutt to timeout any socket read operation (e.g. SSL_read) after
+ ** this many seconds. A zero (default) or negative value causes Mutt to wait
+ ** indefinitely for the read to complete.
+ */
{ "record", DT_PATH, R_NONE, {.p=}, {.p="~/sent"} },
/*
** .pp
diff --git a/mutt_socket.c b/mutt_socket.c
index 3e192072..9468319e 100644
--- a/mutt_socket.c
+++ b/mutt_socket.c
@@ -433,6 +433,14 @@ static int socket_connect (int fd, struct sockaddr* sa)
save_errno = 0;
+ if (ReceiveTimeout > 0)
+ {
+struct timeval tv = { ReceiveTimeout, 0 };
+if (setsockopt (fd, SOL_SOCKET, SO_RCVTIMEO, (char *), sizeof(tv)) < 0)
+ dprint (1, (debugfile, "socket_connect: error setting receive timeout
(%s)\n",
+ strerror(errno)));
+ }
+
if (connect (fd, sa, sa_size) < 0)
{
save_errno = errno;
--
2.34.1
