Thread

1. Unsafe threading in syslogger on Windows

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-08T14:59:28Z

   On Windows, syslogger uses two threads. The main thread loops and polls
   if any SIGHUPs have been received or if the log file needs to be
   rotated. Another thread, "pipe thread", does ReadFile() on the pipe that
   other processes send their log messages to. ReadFile() blocks, and
   whenever new data arrives, it is processed in the pipe thread.
   
   Both threads use palloc()/pfree(), which are not thread-safe :-(.
   
   It's hard to trigger a crash because the main thread mostly just sleeps,
   and the pipe thread only uses palloc()/pfree() when it receives chunked
   messages, larger than 512 bytes. Browsing the CVS history, this was made
   visibly broken by the patch that introduced the message chunking. Before
   that the pipe thread just read from the pipe and wrote to the log file,
   which was safe. It has always used ereport() to report read errors,
   though, which can do palloc(), but there shouldn't normally be any read
   errors.
   
   I chatted with Magnus about this, and he suggested using a Windows
   critical section to make sure that only one of the threads is active at
   a time. That seems suitable for back-porting, but I'd like to get rid of
   this threading in CVS head, it seems too error-prone.
   
   The reason it uses threads like this on Windows is explained in the
   comments:
   > /*
   >  * Worker thread to transfer data from the pipe to the current logfile.
   >  *
   >  * We need this because on Windows, WaitForSingleObject does not work on
   >  * unnamed pipes: it always reports "signaled", so the blocking ReadFile won't
   >  * allow for SIGHUP; and select is for sockets only.
   >  */
   
   But Magnus pointed out that our pgpipe() implementation on Windows
   actually creates a pair of sockets instead of pipes, for exactly that
   reason, so that you can use select() on the returned file descriptor.
   For some reason syslogger explicitly doesn't use pgpipe() on Windows,
   though, but calls CreatePipe(). I don't see any explanation why.
   
   I'm going to see what happens if I remove all the #ifdef WIN32 blocks in
   syslogger, and let it use pgpipe() and select() instead of the extra thread.
   
   Thoughts?
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

2. Re: Unsafe threading in syslogger on Windows

   Andrew Dunstan <andrew@dunslane.net> — 2010-04-08T15:52:34Z

   
   Heikki Linnakangas wrote:
   > On Windows, syslogger uses two threads. The main thread loops and polls
   > if any SIGHUPs have been received or if the log file needs to be
   > rotated. Another thread, "pipe thread", does ReadFile() on the pipe that
   > other processes send their log messages to. ReadFile() blocks, and
   > whenever new data arrives, it is processed in the pipe thread.
   >
   > Both threads use palloc()/pfree(), which are not thread-safe :-(.
   >
   > It's hard to trigger a crash because the main thread mostly just sleeps,
   > and the pipe thread only uses palloc()/pfree() when it receives chunked
   > messages, larger than 512 bytes. Browsing the CVS history, this was made
   > visibly broken by the patch that introduced the message chunking. Before
   > that the pipe thread just read from the pipe and wrote to the log file,
   > which was safe. It has always used ereport() to report read errors,
   > though, which can do palloc(), but there shouldn't normally be any read
   > errors.
   >
   > I chatted with Magnus about this, and he suggested using a Windows
   > critical section to make sure that only one of the threads is active at
   > a time. That seems suitable for back-porting, but I'd like to get rid of
   > this threading in CVS head, it seems too error-prone.
   >
   > The reason it uses threads like this on Windows is explained in the
   > comments:
   >   
   >> /*
   >>  * Worker thread to transfer data from the pipe to the current logfile.
   >>  *
   >>  * We need this because on Windows, WaitForSingleObject does not work on
   >>  * unnamed pipes: it always reports "signaled", so the blocking ReadFile won't
   >>  * allow for SIGHUP; and select is for sockets only.
   >>  */
   >>     
   >
   > But Magnus pointed out that our pgpipe() implementation on Windows
   > actually creates a pair of sockets instead of pipes, for exactly that
   > reason, so that you can use select() on the returned file descriptor.
   > For some reason syslogger explicitly doesn't use pgpipe() on Windows,
   > though, but calls CreatePipe(). I don't see any explanation why.
   >
   > I'm going to see what happens if I remove all the #ifdef WIN32 blocks in
   > syslogger, and let it use pgpipe() and select() instead of the extra thread.
   >
   >
   >   
   
   
   Sounds reasonable. Let's see how big the changes are on HEAD. I'm not 
   sure it's worth creating a different smaller fix for the back branches.
   
   cheers
   
   andrew
   
   
   
   

3. Re: Unsafe threading in syslogger on Windows

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-08T22:14:12Z

   Andrew Dunstan wrote:
   > Heikki Linnakangas wrote:
   >> I'm going to see what happens if I remove all the #ifdef WIN32 blocks in
   >> syslogger, and let it use pgpipe() and select() instead of the extra
   >> thread.
   >
   > Sounds reasonable. Let's see how big the changes are on HEAD. I'm not
   > sure it's worth creating a different smaller fix for the back branches.
   
   I tried that, and got a crash somewhere in the code that inherits the
   syslogger pipe/socket to the child process. I don't understand why, and
   I don't feel like debugging any deeper into that right now. If you or
   someone else wants to give it a shot, that would be good. If not, I
   might try again some other day after sleeping over it.
   
   Anyway, here's the patch I had in mind for back-branches.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   

4. Re: Unsafe threading in syslogger on Windows

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-16T09:54:17Z

   Heikki Linnakangas wrote:
   > Andrew Dunstan wrote:
   >> Heikki Linnakangas wrote:
   >>> I'm going to see what happens if I remove all the #ifdef WIN32 blocks in
   >>> syslogger, and let it use pgpipe() and select() instead of the extra
   >>> thread.
   >> Sounds reasonable. Let's see how big the changes are on HEAD. I'm not
   >> sure it's worth creating a different smaller fix for the back branches.
   > 
   > I tried that, and got a crash somewhere in the code that inherits the
   > syslogger pipe/socket to the child process. I don't understand why, and
   > I don't feel like debugging any deeper into that right now. If you or
   > someone else wants to give it a shot, that would be good. If not, I
   > might try again some other day after sleeping over it.
   > 
   > Anyway, here's the patch I had in mind for back-branches.
   
   I've committed that patch to all branches now. In the long run it would
   be good to get rid of the extra thread, but I'll leave that to someone
   else with more Windows skills.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com