bugPipeline manipulation library - Bugs: bug #34379, Thread safety issues

 
 

bug #34379: Thread safety issues

Submitter:  None
Submitted:  Sun 25 Sep 2011 06:13:20 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  cjwatson
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Mon 26 Sep 2011 10:32:59 PM UTC, comment #4: 

Hi,

No problem. :-) I've attached a pure pthreads version:

$ gcc -o test -lpthread -lpipeline main.c
$ ./test
PRODUCED SOME WORK
PRODUCED SOME WORK
PRODUCED SOME WORK
CONSUMED SOME WORK
PRODUCED SOME WORK
CONSUMED SOME WORK
PRODUCED SOME WORK
./test: waitpid failed: No child processes

Thanks for working on this, libpipeline rocks.

(file #24029)

Anonymous
Mon 26 Sep 2011 02:41:02 PM UTC, comment #3: 

OK.  This is a good start, but I am going to need to remove dependencies on other build systems and dependencies on boost before I can integrate it into libpipeline's test suite (although this certainly helps with informal testing), and I'd prefer it to be pure C (with pthreads) if possible.  However, if you don't have time to do that, that's fine - this is good enough for informally validating my work and I can probably take the more formal parts of test suite integration from here.

I've begun work on the libpipeline work required here.  Switching to pthread_sigmask is of course straightforward.  The static variables are more work.

Just so I don't forget this, I'll mention it here: the reason why I have a SIGCHLD handler that actually reaps child processes is because otherwise it doesn't work to have multiple pipelines connected together using pipeline_connect and pipeline_pump.  If you don't reap a child process when it exits, its standard file descriptors aren't closed unless it does so explicitly, which means that a process reading output from it won't get an end-of-file indication; this typically leads to deadlock.

However, as you mention, the static variables aren't thread-safe.  In fact, they aren't even async-signal-safe, and I've been skating over that particular piece of thin ice for a while.  I'm trying to think of a simultaneously thread-safe and async-signal-safe way to communicate an arbitrary amount of data from a signal handler to the main body of a thread, and currently failing.  I might have to say that child process reaping only works properly if you run pipeline_pump or pipeline_wait ...

Colin Watson <cjwatson>
Group administrator
Mon 26 Sep 2011 01:25:34 AM UTC, comment #2: 

Hi,

Thanks for the followup! As you may have noticed, I was able to work around it by forking in synconv, so now I'm 100% sure the race in libpipeline is the culprit. :-)

I'm attaching a very minimalistic test case. To build it, you'll need:

  • cmake + pkg-config
  • libboost-dev
  • libboost-thread-dev


Then you can run:

$ cd /path/to/extracted
$ mkdir build
$ cd build
$ cmake ..
$ make
$ ./test
PRODUCED SOME WORK
PRODUCED SOME WORK
PRODUCED SOME WORK
CONSUMED SOME WORK
PRODUCED SOME WORK
./test: waitpid failed: No child processes
test: /usr/include/boost/thread/pthread/mutex.hpp:47: boost::mutex::~mutex(): Assertion `!pthread_mutex_destroy(&m)' failed.
Aborted

Please disregard the boost::mutex assertion, it's probably related to the static initialization of mutexes + my abort() call.

Since this is a classic race condition, you may not be able to reproduce it consistently. Here, with a 2-core CPU, I've been able to reproduce it every time after 3 or 4 seconds. You may want to tune NUM_THREADS to make it easier to reproduce on your machine.

Please let me know if you want it converted to pure pthreads (or even pure C), shouldn't be too hard. Also, please let me know of anything else you'd think could be helpful.

Thanks in advance,

(file #24022)

Anonymous
Sun 25 Sep 2011 08:46:49 PM UTC, comment #1: 

I avoid threading in my own programs (preferring multiprocessing or state machines as appropriate), so until now thread-safety hasn't been a priority for libpipeline.  However, I'd be happy to work on this, since it sounds as though the changes should be reasonably small, especially with the aid of gnulib.  It would really help if you could supply a test case for libpipeline's test suite that exercises the failing situations as thoroughly as possible; I guess you might be able to extract something like that out of synconv, while I'd have to write it from scratch.  Do you think you might be able to do that?

Ignoring SIGCHLD would require a rewrite of all the wait status handling code; it took a very long time to get that right the first time, so I really don't want to go anywhere near that unless I have no conceivable alternative.  I don't think that kind of upheaval should be necessary.

Thanks for your interest in libpipeline!

Colin Watson <cjwatson>
Group administrator
Sun 25 Sep 2011 06:13:20 PM UTC, original submission:  

Hi,

I'm facing some thread-safety issues in my libpipeline-based project[1], which I think are related to libpipeline. It looks like pipeline.c uses static variables and messes with the process signal mask. It might be possible to use pthread_sigmask and make those static variables thread-local.

At least pipeline_wait_all and the signal handlers should be made thread-safe. If the setting up of the pipeline would remain thread-unsafe, it wouldn't be a big deal.

So, what do you think? Is there a workaround you can think of? Is it always necessary to handle SIGCHLD? As far as I know, if you ignore SIGCHLD, your children won't become zombies anymore in Linux/BSD.

In synconv, we use libpipeline to create the transcoding pipeline (say, decoding with flac and encoding with lame). So we launch N different threads for each of the files that need to be transcoded. The threads block on pipeline_wait. If I use more than a single thread, I occasionally get an error message such as:

synconv: waitpid failed: No child processes

If you think going with TLS and maybe a lock or two isn't the way to go, I think I'll have to fork into another process to use libpipeline.

Thanks,

[1]: https://github.com/fernandotcl/synconv

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #24029:  main.c added by None (2KiB - text/x-csrc - pure C and pthreads testcase)
file #24022:  testcase.tar.gz added by None (1KiB - application/x-gzip - Minimalistic testcase)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by cjwatson (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-09-26 None Attached File- Added main.c, #24029
    2011-09-26 cjwatson Assigned toNone cjwatson
    2011-09-26 None Attached File- Added testcase.tar.gz, #24022

    Back to the top

    Powered by Savane 3.14-f13d.
    Corresponding source code