Re: SIGPIPE handling

Attached is my idea for implementing safe SIGPIPE in threaded apps.  The
code has the same libpq behavior if not compiled using
--enable-thread-safety.

If compiled with that option, an app wanting to define its own SIGPIPE
handler has to do so before connecting to a database.  On first
connection, the code checks to see if there is a SIGPIPE handler, and if
not, installs its own, and creates a thread-local variable.  Then, on
each send(), it sets, calls send(), then clears the thread-local
variable.  The SIGPIPE handler checks the thread-local variable and
either ignores or exits depending on whether it was in send().

Right now the thread-local variable is static to the file, but we could
export it as a boolean so custom SIGPIPE handlers could check it and
take action or ignore the signal just like our code.  Not sure if that
is a good idea or not.  In fact, even cleaner, we could create a
function that allows users to define their own SIGPIPE handler and it
would be called only when not called by libpq send(), and it would work
safely for threaded apps.

I think the big problem with my approach is that it requires special
custom SIGPIPE handler code even if the app isn't multi-threaded but
libpq is compiled as multi-threaded.

Another idea is to create PQsigpipefromsend() that returns true/false
depending on whether the SIGPIPE was from libpq's send().  It could be a
global variable set/cleared in non-threaded libpq and a thread-local
variable in threaded libpq.  It would allow the same API/behavior for
both libpq versions and all custom SIGPIPE handlers using libpq would
have to check it.

The one good thing about the patch is that it ignores send() SIGPIPE,
and gives default SIG_DFL behavior for libpq apps with no special app
coding, with the downside of requiring extra cost for custom SIGPIPE
handlers.

---------------------------------------------------------------------------

Manfred Spraul wrote:
> Bruce Momjian wrote:
>
> >OK, I know you had a flag for pgbench, and that doesn't use threads.
> >What speedup do you see there?
> >
> >
> Tiny. I added the flag to check that my implementation works, not as a
> benchmark tool.
>
> >I would not expect a library to require me to do something in my code to
> >be thread-safe --- either it is or it isn't.
> >
> The library is thread-safe. Just the SIGPIPE handling differs:
> - single thread: handled by libpq.
> - multi thread: caller must handle SIGPIPE for libpq.
> Rationale: posix is broken. Per-thread signal handling is too ugly to
> think about.
>
> >Again, let's get it working perfect if they say they are going to use
> >threads with libpq.  Does it work OK if the app doesn't use threading?
> >
> >
> No. pthread_sigmask is part of libpthread - libpq would have to link
> unconditionally against libpthread. Or use __attribute__((weak,
> alias())), but that would only work with gcc.
>
> >Does sigpending/sigwait work efficiently for threads?  Another idea is
> >to go with a thread-local storage boolean for each thread, and check
> >that in a signal handler we install.
> >
> I think installing a signal handler is not an option - libpq is a
> library, the signal handler is global.
>
> >  Seems synchronous signals like
> >SIGPIPE are delivered to the thread that invoked them, and we can check
> >thread-local storage to see if we were in a send() loop at the time of
> >signal delivery.
> >
> >
> IMHO way to fragile.

--
  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
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.263
diff -c -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    18 Nov 2003 22:42:44 -0000
***************
*** 43,48 ****
--- 43,52 ----
  #include <arpa/inet.h>
  #endif

+ #if defined(USE_THREADS) && !defined(WIN32)
+ #include <pthread.h>
+ #endif
+
  #include "libpq/ip.h"
  #include "mb/pg_wchar.h"

***************
*** 66,72 ****
  #define DefaultSSLMode    "disable"
  #endif

-
  /* ----------
   * Definition of the conninfo parameters and their fallback resources.
   *
--- 70,75 ----
***************
*** 198,203 ****
--- 201,207 ----
  static char *PasswordFromFile(char *hostname, char *port, char *dbname,
                   char *username);

+
  /*
   *        Connecting to a Database
   *
***************
*** 881,886 ****
--- 885,895 ----
      struct addrinfo hint;
      const char *node = NULL;
      int            ret;
+ #if defined(USE_THREADS) && !defined(WIN32)
+     static pthread_once_t check_sigpipe_once = PTHREAD_ONCE_INIT;
+
+     pthread_once(&check_sigpipe_once, check_sigpipe_handler);
+ #endif

      if (!conn)
          return 0;
***************
*** 3172,3174 ****
--- 3181,3184 ----

  #undef LINELEN
  }
+
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.32
diff -c -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    18 Nov 2003 22:42:45 -0000
***************
*** 106,111 ****
--- 106,115 ----
  #include <arpa/inet.h>
  #endif

+ #if defined(USE_THREADS) && !defined(WIN32)
+ #include <pthread.h>
+ #endif
+
  #ifndef HAVE_STRDUP
  #include "strdup.h"
  #endif
***************
*** 142,147 ****
--- 146,156 ----
  static SSL_CTX *SSL_context = NULL;
  #endif

+ #if defined(USE_THREADS) && !defined(WIN32)
+ static void nonsend_sigpipe_handler(int signo);
+ static pthread_key_t in_send;
+ #endif
+
  /* ------------------------------------------------------------ */
  /*                         Hardcoded values                        */
  /* ------------------------------------------------------------ */
***************
*** 347,355 ****
  {
      ssize_t        n;

! #ifndef WIN32
      pqsigfunc    oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
  #endif

  #ifdef USE_SSL
      if (conn->ssl)
--- 356,368 ----
  {
      ssize_t        n;

! #if !defined(WIN32)
! #if defined(USE_THREADS)
!     pthread_setspecific(in_send, "1");
! #else
      pqsigfunc    oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
  #endif
+ #endif

  #ifdef USE_SSL
      if (conn->ssl)
***************
*** 407,415 ****
  #endif
          n = send(conn->sock, ptr, len, 0);

! #ifndef WIN32
      pqsignal(SIGPIPE, oldsighandler);
  #endif

      return n;
  }
--- 420,432 ----
  #endif
          n = send(conn->sock, ptr, len, 0);

! #if !defined(WIN32)
! #if defined(USE_THREADS)
!     pthread_setspecific(in_send, "0");
! #else
      pqsignal(SIGPIPE, oldsighandler);
  #endif
+ #endif

      return n;
  }
***************
*** 1042,1044 ****
--- 1059,1099 ----
  }

  #endif   /* USE_SSL */
+
+
+ #if defined(USE_THREADS) && !defined(WIN32)
+ /*
+  *    Check SIGPIPE handler and perhaps install our own.
+  */
+ void check_sigpipe_handler(void)
+ {
+     pqsigfunc pipehandler;
+
+     /*
+      *    If the app hasn't set a SIGPIPE handler, define our own
+      *    that ignores SIGPIPE on libpq send() and does SIG_DFL
+      *    for other SIGPIPE cases.
+      */
+     pipehandler = pqsignalinquire(SIGPIPE);
+     if (pipehandler == SIG_DFL || pipehandler == SIG_ERR)
+     {
+         /* Do this first because the signal handler can be called anytime */
+         pthread_key_create(&in_send, NULL);
+         pqsignal(SIGPIPE, nonsend_sigpipe_handler);
+     }
+ }
+
+ /*
+  *    Threaded SIGPIPE signal handler
+  */
+ void
+ nonsend_sigpipe_handler(int signo)
+ {
+     char *is_in_send = pthread_getspecific(in_send);
+
+     /* If we have gotten a SIGPIPE outside send(), exit */
+     if (is_in_send == NULL || *is_in_send == '0')
+         exit(128 + SIGPIPE);
+ }
+ #endif
+
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.82
diff -c -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    18 Nov 2003 22:42:46 -0000
***************
*** 29,35 ****
  #include <sys/time.h>
  #endif

-
  #if defined(WIN32) && (!defined(ssize_t))
  typedef int ssize_t;            /* ssize_t doesn't exist in VC (at least
                                   * not VC6) */
--- 29,34 ----
***************
*** 442,447 ****
--- 441,449 ----
  extern void pqsecure_close(PGconn *);
  extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len);
  extern ssize_t pqsecure_write(PGconn *, const void *ptr, size_t len);
+ #if defined(USE_THREADS) && !defined(WIN32)
+ extern void check_sigpipe_handler(void);
+ #endif

  /*
   * this is so that we can check if a connection is non-blocking internally
Index: src/interfaces/libpq/pqsignal.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/pqsignal.c,v
retrieving revision 1.17
diff -c -c -r1.17 pqsignal.c
*** src/interfaces/libpq/pqsignal.c    4 Aug 2003 02:40:20 -0000    1.17
--- src/interfaces/libpq/pqsignal.c    18 Nov 2003 22:42:46 -0000
***************
*** 40,42 ****
--- 40,61 ----
      return oact.sa_handler;
  #endif   /* !HAVE_POSIX_SIGNALS */
  }
+
+ pqsigfunc
+ pqsignalinquire(int signo)
+ {
+ #if !defined(HAVE_POSIX_SIGNALS)
+     pqsigfunc old;
+      old = signal(SIGPIPE, SIG_IGN);
+     signal(SIGPIPE, old);
+     return old;
+ #else
+     struct sigaction oact;
+
+     if (sigaction(SIGPIPE, NULL, &oact) != 0)
+            return SIG_ERR;
+     return oact.sa_handler;
+ #endif   /* !HAVE_POSIX_SIGNALS */
+
+
+ }
Index: src/interfaces/libpq/pqsignal.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/pqsignal.h,v
retrieving revision 1.15
diff -c -c -r1.15 pqsignal.h
*** src/interfaces/libpq/pqsignal.h    4 Aug 2003 02:40:20 -0000    1.15
--- src/interfaces/libpq/pqsignal.h    18 Nov 2003 22:42:46 -0000
***************
*** 24,27 ****
--- 24,29 ----

  extern pqsigfunc pqsignal(int signo, pqsigfunc func);

+ extern pqsigfunc pqsignalinquire(int signo);
+
  #endif   /* PQSIGNAL_H */