[BioRuby] Thread-safety of alignment

[Naohisa GOTO]

Hi Andrew Grimm,

This is not the Tempfile bug, but the problem of fork with threads.

Currently, to call a command safely without escaping of command-line
string, bioruby internally uses fork in UNIX.

When fork is executed, whole data of the process is copied to the child
process, including Ruby threads and finalizers of Tempfile objects.
In the child process, when the running thread is switched, unexpected
behavior may occur, for example, executing another ClustalW with the same
output file name of the parent process, temporary files are removed by
finalizers of child process when the ClustalW is regarded as finished in
the child process but not in the parent process.

The patch below can fix, or can reduce the problem.
-------------------------------------------------------------------
diff --git a/lib/bio/command.rb b/lib/bio/command.rb
index 4f3ac94..ebd9cc5 100644
--- a/lib/bio/command.rb
+++ b/lib/bio/command.rb
@@ -196,12 +196,15 @@ module Command
   def call_command_fork(cmd, options = {})
     dir = options[:chdir]
     cmd = safe_command_line_array(cmd)
+    tc, Thread.critical = Thread.critical, true
     IO.popen("-", "r+") do |io|
       if io then
         # parent
+        Thread.critical = tc
         yield io
       else
         # child
+        GC.disable
         # chdir to options[:chdir] if available
         begin
           Dir.chdir(dir) if dir

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

Note that the patch does not work with Ruby 1.9 because Thread.critical
is removed in Ruby 1.9.

In Ruby 1.9.1, IO.popen is improved to get command-line as an array
without calling shell, and the problem of string escaping is completely
resolved. Changes supporting Ruby 1.9.1 will soon be available in my
GitHub repository.

Naohisa Goto
ngoto at gen-info.osaka-u.ac.jp / ng at bioruby.org


On Wed, 27 Jan 2010 14:07:18 +1100
Andrew Grimm <andrew.j.grimm at gmail.com> wrote:

> Hi Naohisa Goto,
> 
> On Wed, Jan 27, 2010 at 2:00 AM, Naohisa GOTO
> <ngoto at gen-info.osaka-u.ac.jp> wrote:
> > Hi Andrew,
> >
> > On Tue, 26 Jan 2010 23:12:35 +1100
> > Andrew Grimm <andrew.j.grimm at gmail.com> wrote:
> >
> >> Hi Naohisa Goto,
> >>
> >> I tried creating a new factory in each thread, but I sometimes (but
> >> not always) have errors.
> >
> > Please show ruby version and BioRuby version.
> >  % ruby -v
> >  % ruby -rbio -e 'puts Bio::BIORUBY_VERSION_ID'
> > (If you are using BioRuby 1.2.1 or earlier,
> >  % ruby -rbio -e 'p Bio::BIORUBY_VERSION'
> > )
> >
> 
> I'm running ruby 1.8.7 (2008-08-11 patchlevel 72) and bioruby 1.4.0.
> 
> >> Is the code in http://github.com/agrimm/bioruby-alignment-threading-replication/blob/master/test/test_multithreaded_alignment.rb
> >> correct? Does it cause problems for anyone else?
> >
> > The "rescue RuntimeError" in line 15 may hide problems.
> > In my environment, it seems that the RuntimeError is raised
> > in lib/bio/alignment.rb. The error message I observed
> > without the rescue was
> > "alignment result is inconsistent with input data",
> > and output file created by Clustalw was unexpectedly empty.
> > It might be a bug of Tempfile in Ruby, but not sure.
> >
> > With Ruby 1.8.7, errors are observed in some times.
> >  % ruby -v
> >  ruby 1.8.7 (2010-01-10 patchlevel 249) [i686-linux]
> >  ruby 1.8.7 (2009-04-08 patchlevel 160) [i686-linux]
> >  ruby 1.8.7 (2008-08-11 patchlevel 72) [i486-linux]
> >
> > With Ruby 1.9.1-p378, no errors when I executed several times.
> >  % ruby -v
> >  ruby 1.9.1p378 (2010-01-10 revision 26273) [i686-linux]
> >
> 
> I suspect errors may occur on earlier versions of ruby 1.9.1.
> 
> >> Some of the errors I get include the ones seen at http://gist.github.com/286775
> >
> > The message "ERROR: Multiple sequences found with same name
> > (found 0 at least twice)!" is reported by ClustalW, and
> > it indicates incorrect input file sequence names. Maybe
> > two file contents are unexpectedly concatenated or mixed
> > possibly due to a bug of Tempfile, but not sure.
> >
> >> It's possible that the issues are caused by problems in tempfile
> >> itself (which may have been fixed in August 2009 according to the
> >> changelog).
> >
> > Another possibility is resource limits of the machine:
> > the number of child processes, total memory size, etc.
> > If exceeding limits, new child clustalw process could
> > not be started, or running clustalw processes might be
> > killed. This also causes void or truncated result files,
> > and leads to ruby-level errors.
> >
> 
> Thanks for that suggestion. I re-ran the test using only 5 threads in
> the new gist http://gist.github.com/287499
> 
> > Thanks,
> >
> > Naohisa Goto
> > ngoto at gen-info.osaka-u.ac.jp / ng at bioruby.org
> >
> >>
> >> Thanks,
> >>
> >> Andrew
> >>
> >> On Thu, Jan 21, 2010 at 12:50 AM, Naohisa GOTO
> >> <ngoto at gen-info.osaka-u.ac.jp> wrote:
> >> > Hi,
> >> >
> >> > On Wed, 20 Jan 2010 23:09:19 +1100
> >> > Andrew Grimm <andrew.j.grimm at gmail.com> wrote:
> >> >
> >> >> Is alignment intended to be thread-safe in bioruby? If so, should I
> >> >> use the same alignment factory between threads, or a separate one in
> >> >> each thread?
> >> >
> >> > It is not confirmed to be thread-safe, so it is safe to use
> >> > separate one in each thread.
> >> >
> >> > Currently, in BioRuby, manipulating the same object from different
> >> > threads is not intended. When manipulating the same object from
> >> > different threads is needed, using mutex is recommended.
> >> >
> >> > For library developers, it is encouraged to write thread-safe
> >> > code if possible, but not mandatory.
> >> >
> >> > Naohisa Goto
> >> > ngoto at gen-info.osaka-u.ac.jp / ng at bioruby.org
> >> >
> >> >>
> >> >> Andrew