Обсуждение: SIGPIPE handling, take two.
pqsecure_write tries to catch SIGPIPE signals generated by network
disconnects by setting the signal handler to SIG_IGN. The current
approach causes several problems:
- it always sets SA_RESTART when it restores the old handler.
- it's not reliable for multi threaded apps, because another thread
could change the signal handler inbetween.
- it's slow, because after setting a signal handler to SIG_IGN the
kernel must enumerate all threads and clear all pending signals (at
least FreeBSD-5.1 and linux-2.6 do that. Earlier linux kernels don't -
their signal handling is known to be broken for multithreaded apps).
Initially I proposed a new option for PQconnectdb, but Tom didn't like
that. The attached patch autodetects if it should set the signal
handler, Tom proposed that. The code doesn't try to check if the signal
is "handled" by blocking it, because I haven't figured out how to check
that: sigprocmask is undefined for multithreaded apps and calling
pthread_sigmask would force every libpq user to link against libpthread.
--
Manfred
? src/interfaces/libpq/libpq.so.3.1
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.263
diff -c -r1.263 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 18 Oct 2003 05:02:06 -0000 1.263
--- src/interfaces/libpq/fe-connect.c 2 Nov 2003 18:29:40 -0000
***************
*** 41,46 ****
--- 41,47 ----
#include <netinet/tcp.h>
#endif
#include <arpa/inet.h>
+ #include <signal.h>
#endif
#include "libpq/ip.h"
***************
*** 951,956 ****
--- 952,983 ----
else if (conn->sslmode[0] == 'a') /* "allow" */
conn->wait_ssl_try = true;
#endif
+ /*
+ * Autodetect SIGPIPE signal handling:
+ * The default action per Unix spec is kill current process and
+ * that's not acceptable. If the current setting is not the default,
+ * then assume that the caller knows what he's doing and leave the
+ * signal handler unchanged. Otherwise set the signal handler to
+ * SIG_IGN around each send() syscall. Unfortunately this is both
+ * unreliable and slow for multithreaded apps.
+ */
+ conn->do_sigaction = true;
+ #if !defined(HAVE_POSIX_SIGNALS)
+ {
+ pqsigfunc old;
+ old = signal(SIGPIPE, SIG_IGN);
+ if (old != SIG_DFL)
+ conn->do_sigaction = false;
+ signal(SIGPIPE, old);
+ }
+ #else
+ {
+ struct sigaction oact;
+
+ if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL)
+ conn->do_sigaction = false;
+ }
+ #endif /* !HAVE_POSIX_SIGNALS */
/*
* Set up to try to connect, with protocol 3.0 as the first attempt.
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.32
diff -c -r1.32 fe-secure.c
*** src/interfaces/libpq/fe-secure.c 29 Sep 2003 16:38:04 -0000 1.32
--- src/interfaces/libpq/fe-secure.c 2 Nov 2003 18:29:41 -0000
***************
*** 348,354 ****
ssize_t n;
#ifndef WIN32
! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
#endif
#ifdef USE_SSL
--- 348,357 ----
ssize_t n;
#ifndef WIN32
! pqsigfunc oldsighandler = NULL;
!
! if (conn->do_sigaction)
! oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
#endif
#ifdef USE_SSL
***************
*** 408,414 ****
n = send(conn->sock, ptr, len, 0);
#ifndef WIN32
! pqsignal(SIGPIPE, oldsighandler);
#endif
return n;
--- 411,418 ----
n = send(conn->sock, ptr, len, 0);
#ifndef WIN32
! if (conn->do_sigaction)
! pqsignal(SIGPIPE, oldsighandler);
#endif
return n;
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.82
diff -c -r1.82 libpq-int.h
*** src/interfaces/libpq/libpq-int.h 5 Sep 2003 02:08:36 -0000 1.82
--- src/interfaces/libpq/libpq-int.h 2 Nov 2003 18:29:42 -0000
***************
*** 329,334 ****
--- 329,335 ----
char peer_dn[256 + 1]; /* peer distinguished name */
char peer_cn[SM_USER + 1]; /* peer common name */
#endif
+ bool do_sigaction; /* set SIGPIPE to SIG_IGN around every send() call */
/* Buffer for current error message */
PQExpBufferData errorMessage; /* expansible string */
I think this is the patch I like. It does the auto-detect handling as I
hoped. I will just do the doc updates to mention it.
My only issue is that this is per-connection, while I think you have to
create a global variable that defaults to false, and on first connect,
check, and not after. Based on the code below, a second connection
would have the SIGPIPE signal set to SIG_IGN, not SIG_DEF, and you
would be back to setting SIG_IGN around each send, even though it was
already set.
Are others OK with this too?
---------------------------------------------------------------------------
Manfred Spraul wrote:
> pqsecure_write tries to catch SIGPIPE signals generated by network
> disconnects by setting the signal handler to SIG_IGN. The current
> approach causes several problems:
> - it always sets SA_RESTART when it restores the old handler.
> - it's not reliable for multi threaded apps, because another thread
> could change the signal handler inbetween.
> - it's slow, because after setting a signal handler to SIG_IGN the
> kernel must enumerate all threads and clear all pending signals (at
> least FreeBSD-5.1 and linux-2.6 do that. Earlier linux kernels don't -
> their signal handling is known to be broken for multithreaded apps).
>
> Initially I proposed a new option for PQconnectdb, but Tom didn't like
> that. The attached patch autodetects if it should set the signal
> handler, Tom proposed that. The code doesn't try to check if the signal
> is "handled" by blocking it, because I haven't figured out how to check
> that: sigprocmask is undefined for multithreaded apps and calling
> pthread_sigmask would force every libpq user to link against libpthread.
>
> --
> Manfred
> ? src/interfaces/libpq/libpq.so.3.1
> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.263
> diff -c -r1.263 fe-connect.c
> *** src/interfaces/libpq/fe-connect.c 18 Oct 2003 05:02:06 -0000 1.263
> --- src/interfaces/libpq/fe-connect.c 2 Nov 2003 18:29:40 -0000
> ***************
> *** 41,46 ****
> --- 41,47 ----
> #include <netinet/tcp.h>
> #endif
> #include <arpa/inet.h>
> + #include <signal.h>
> #endif
>
> #include "libpq/ip.h"
> ***************
> *** 951,956 ****
> --- 952,983 ----
> else if (conn->sslmode[0] == 'a') /* "allow" */
> conn->wait_ssl_try = true;
> #endif
> + /*
> + * Autodetect SIGPIPE signal handling:
> + * The default action per Unix spec is kill current process and
> + * that's not acceptable. If the current setting is not the default,
> + * then assume that the caller knows what he's doing and leave the
> + * signal handler unchanged. Otherwise set the signal handler to
> + * SIG_IGN around each send() syscall. Unfortunately this is both
> + * unreliable and slow for multithreaded apps.
> + */
> + conn->do_sigaction = true;
> + #if !defined(HAVE_POSIX_SIGNALS)
> + {
> + pqsigfunc old;
> + old = signal(SIGPIPE, SIG_IGN);
> + if (old != SIG_DFL)
> + conn->do_sigaction = false;
> + signal(SIGPIPE, old);
> + }
> + #else
> + {
> + struct sigaction oact;
> +
> + if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL)
> + conn->do_sigaction = false;
> + }
> + #endif /* !HAVE_POSIX_SIGNALS */
>
> /*
> * Set up to try to connect, with protocol 3.0 as the first attempt.
> Index: src/interfaces/libpq/fe-secure.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
> retrieving revision 1.32
> diff -c -r1.32 fe-secure.c
> *** src/interfaces/libpq/fe-secure.c 29 Sep 2003 16:38:04 -0000 1.32
> --- src/interfaces/libpq/fe-secure.c 2 Nov 2003 18:29:41 -0000
> ***************
> *** 348,354 ****
> ssize_t n;
>
> #ifndef WIN32
> ! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
> #endif
>
> #ifdef USE_SSL
> --- 348,357 ----
> ssize_t n;
>
> #ifndef WIN32
> ! pqsigfunc oldsighandler = NULL;
> !
> ! if (conn->do_sigaction)
> ! oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
> #endif
>
> #ifdef USE_SSL
> ***************
> *** 408,414 ****
> n = send(conn->sock, ptr, len, 0);
>
> #ifndef WIN32
> ! pqsignal(SIGPIPE, oldsighandler);
> #endif
>
> return n;
> --- 411,418 ----
> n = send(conn->sock, ptr, len, 0);
>
> #ifndef WIN32
> ! if (conn->do_sigaction)
> ! pqsignal(SIGPIPE, oldsighandler);
> #endif
>
> return n;
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.82
> diff -c -r1.82 libpq-int.h
> *** src/interfaces/libpq/libpq-int.h 5 Sep 2003 02:08:36 -0000 1.82
> --- src/interfaces/libpq/libpq-int.h 2 Nov 2003 18:29:42 -0000
> ***************
> *** 329,334 ****
> --- 329,335 ----
> char peer_dn[256 + 1]; /* peer distinguished name */
> char peer_cn[SM_USER + 1]; /* peer common name */
> #endif
> + bool do_sigaction; /* set SIGPIPE to SIG_IGN around every send() call */
>
> /* Buffer for current error message */
> PQExpBufferData errorMessage; /* expansible string */
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote:
> I think this is the patch I like. It does the auto-detect handling as I
> hoped. I will just do the doc updates to mention it.
>
> My only issue is that this is per-connection, while I think you have to
> create a global variable that defaults to false, and on first connect,
> check, and not after. Based on the code below, a second connection
> would have the SIGPIPE signal set to SIG_IGN, not SIG_DEF, and you
> would be back to setting SIG_IGN around each send, even though it was
> already set.
>
> Are others OK with this too?
I believe that the are some errors on the following code:
#if !defined(HAVE_POSIX_SIGNALS)
{
pqsigfunc old;
old = signal(SIGPIPE, SIG_IGN);
if (old != SIG_DFL)
conn->do_sigaction = false;
signal(SIGPIPE, old);
}
#else
{
struct sigaction oact;
if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL)
conn->do_sigaction = false;
}
#endif /* !HAVE_POSIX_SIGNALS */
the old signal handler is not reinstated in case of
HAVE_POSIX_SIGNAL
May be this sound better:
#if !defined(HAVE_POSIX_SIGNALS)
{
pqsigfunc old;
old = signal(SIGPIPE, SIG_IGN);
if (old != SIG_DFL && old != SIG_ERR)
conn->do_sigaction = false;
if ( old != SIG_ERR )
signal(SIGPIPE, old);
}
#else
{
struct sigaction oact;
int err;
if ( (err = sigaction(SIGPIPE, NULL, &oact)) == 0 &&
oact.sa_handler != SIG_DFL)
conn->do_sigaction = false;
if ( err == 0 )
sigaction(SIGPIPE, &oact, NULL);
}
#endif /* !HAVE_POSIX_SIGNALS */
Regards
Gaetano Mendola
Gaetano Mendola wrote:
> Bruce Momjian wrote:
>
>> I think this is the patch I like. It does the auto-detect handling as I
>> hoped. I will just do the doc updates to mention it.
>>
>> My only issue is that this is per-connection, while I think you have to
>> create a global variable that defaults to false, and on first connect,
>> check, and not after. Based on the code below, a second connection
>> would have the SIGPIPE signal set to SIG_IGN, not SIG_DEF, and you
>> would be back to setting SIG_IGN around each send, even though it was
>> already set.
>>
>> Are others OK with this too?
>
>
> I believe that the are some errors on the following code:
>
> #if !defined(HAVE_POSIX_SIGNALS)
> {
> pqsigfunc old;
> old = signal(SIGPIPE, SIG_IGN);
> if (old != SIG_DFL)
> conn->do_sigaction = false;
> signal(SIGPIPE, old);
> }
> #else
> {
> struct sigaction oact;
>
> if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler !=
> SIG_DFL)
> conn->do_sigaction = false;
> }
> #endif /* !HAVE_POSIX_SIGNALS */
>
> the old signal handler is not reinstated in case of
> HAVE_POSIX_SIGNAL
Forget the message :-)
Gaetano
This message was cancelled from within Mozilla.
This message was cancelled from within Mozilla.
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I think this is the patch I like.
The #if coding is messy and unnecessary. You could do the test as per
the non-POSIX variant using two calls of pqsignal(), and not have any
system dependence here, nor a need for <signal.h>.
regards, tom lane
Oh, forgot to mention ...
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> My only issue is that this is per-connection, while I think you have to
> create a global variable that defaults to false, and on first connect,
> check, and not after.
No, because this patch does not have any global effect on the signal
handling. It might be unnecessary to check per-connection, but it
doesn't hurt, and on grounds of cleanliness I'd prefer to avoid a global
variable.
regards, tom lane
Tom Lane wrote:
>Bruce Momjian <pgman@candle.pha.pa.us> writes:
>
>
>>I think this is the patch I like.
>>
>>
>
>The #if coding is messy and unnecessary. You could do the test as per
>the non-POSIX variant using two calls of pqsignal(), and not have any
>system dependence here, nor a need for <signal.h>.
>
>
What about multithreaded apps?
old = pgsignal(SIPEPIPE, SIG_IGN);
** another thread calls sigaction(SIGPIPE,,);
pgsignal(SIGPIPE, old);
And the signal state is corrupted. What about extending pgsignal:
pgsignal(signo, SIG_ERR);
reads the current signal handler. I'll update my patch.
From your other mail:
>No, because this patch does not have any global effect on the signal
>handling. It might be unnecessary to check per-connection, but it
>doesn't hurt, and on grounds of cleanliness I'd prefer to avoid a global
>variable.
>
>
I agree - global state would require global synchronization.
--
Manfred
Manfred Spraul <manfred@colorfullife.com> writes:
> What about multithreaded apps?
> old = pgsignal(SIPEPIPE, SIG_IGN);
> ** another thread calls sigaction(SIGPIPE,,);
> pgsignal(SIGPIPE, old);
> And the signal state is corrupted.
If other threads are changing the signal state mid-flight, we are
screwed anyway; if not here then later when we are doing sends,
or even more directly because our test here may not reflect reality
later.
I don't think we need to complicate pqsignal's API for this. Instead
we'd better document that SIGPIPE handling has to be set up and kept
stable before doing any libpq operations in a multithread app.
regards, tom lane
Tom Lane wrote:
>I don't think we need to complicate pqsignal's API for this. Instead
>we'd better document that SIGPIPE handling has to be set up and kept
>stable before doing any libpq operations in a multithread app.
>
>
Not reliable.
An app could install it's own signal handler and block SIGPIPE around
all libpq calls. Signal blocking is per-thread. But the SIG_IGN/restore
sequence affects the whole app - PQconnectdb calls would result in
randomly dropped SIGPIPE signals.
--
Manfred
Manfred Spraul <manfred@colorfullife.com> writes:
> ... But the SIG_IGN/restore
> sequence affects the whole app - PQconnectdb calls would result in
> randomly dropped SIGPIPE signals.
Good point. AFAICS we lose anyway if we don't have sigaction()
available, but hopefully any multithreaded platform has sigaction().
I still don't like modifying pqsignal's API though. What I suggest
is adding a function like "pqsignalinquire(signalno)" to pqsignal.c,
defined to return the signal handler without changing it ... that is,
take the system-dependent code you were going to put in fe-connect.c
and put it in pqsignal.c instead.
regards, tom lane