From a9d4eca0ebb412a886cdc736a6e92a16851211a9 Mon Sep 17 00:00:00 2001
From: Adam Lerer <alerer@fb.com>
Date: Wed, 13 Apr 2016 15:16:02 -0700
Subject: propagate errors immediately to prevent deadlocks

---
 README.md                   | 15 +--------------
 test/test-threads-async.lua |  3 ---
 threads.lua                 | 24 ++++++++++++------------
 3 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/README.md b/README.md
index 759303c..633db10 100644
--- a/README.md
+++ b/README.md
@@ -290,14 +290,6 @@ If no such job is available, the main thread of execution will wait (i.e. block)
 In general, this method should not be called, except if one wants to use the [async capabilities](#threads.async) of the Threads class.
 Instead, [synchronize()](#threads.synchronize) should be called to make sure all jobs are executed.
 
-Note that `dojob()` will not raise an error in the main thread, if an error
-occurred in one thread. It will however record errors in a
-buffer. [synchronize()](#threads.synchronize) will raise an error
-summarizing all the errors occuring in all threads. If `dojob()` is called
-specifically, it is thus important to check for errors with
-[haserrors()](#threads.haserror'), and call
-[synchronize()](#threads.synchronize) if a problem occurred.
-
 <a name='threads.synchronize'/>
 #### Threads:synchronize() ####
 This method will call [dojob](#threads.dojob) until all `callbacks` and corresponding `endcallbacks` are executed on the queue and main threads, respectively.
@@ -340,16 +332,11 @@ the function will return `true` if the global thread queue is not full,
 
 Returns `true` if there are still some unfinished jobs running, `false` otherwise.
 
-<a name='threads.haserror'/>
-#### Threads.haserror() ####
-
-Returns `true` if an error occurred in one or several threads, `false` otherwise.
-
 <a name='threads.async'/>
 ### Threads asynchronous mode ###
 
 The methods [acceptsjob()](#threads.acceptsjob) and
-[hasjob()](#threads.hasjob) and [haserror()](#threads.haserror) allow you
+[hasjob()](#threads.hasjob) allow you
 to use the `threads.Threads` in an asynchronous manner, without the need of
 calling [synchronize()](#threads.synchronize). See
 [the asynchronous example](test/test-threads-async.lua) for a typical test
diff --git a/test/test-threads-async.lua b/test/test-threads-async.lua
index 68bcd35..63f4869 100644
--- a/test/test-threads-async.lua
+++ b/test/test-threads-async.lua
@@ -37,9 +37,6 @@ local function get()
    -- is there still something to do?
    if pool:hasjob() then
       pool:dojob() -- yes? do it!
-      if pool:haserror() then -- check for errors
-         pool:synchronize() -- finish everything and throw error
-      end
       return result
    end
 
diff --git a/threads.lua b/threads.lua
index f65d678..8c79827 100644
--- a/threads.lua
+++ b/threads.lua
@@ -34,7 +34,7 @@ function Threads.serialization(name)
 end
 
 function Threads.new(N, ...)
-   local self = {N=N, endcallbacks={n=0}, errors={}, __specific=true, __running=true}
+   local self = {N=N, endcallbacks={n=0}, errors=false, __specific=true, __running=true}
    local funcs = {...}
    local serialize = require(Threads.__serialize)
 
@@ -165,6 +165,7 @@ end
 
 function Threads:dojob()
    checkrunning(self)
+   self.errors = false
    local callstatus, args, endcallbackid, threadid = self.mainqueue:dojob()
    local endcallback = self.endcallbacks[endcallbackid]
    self.endcallbacks[endcallbackid] = nil
@@ -174,10 +175,12 @@ function Threads:dojob()
         function() return endcallback(_unpack(args)) end,
         debug.traceback)
       if not endcallstatus then
-         table.insert(self.errors, string.format('[thread %d endcallback] %s', threadid, msg))
+         self.errors = true
+         error(string.format('[thread %d endcallback] %s', threadid, msg))
       end
    else
-      table.insert(self.errors, string.format('[thread %d callback] %s', threadid, args[1]))
+      self.errors = true
+      error(string.format('[thread %d callback] %s', threadid, args[1]))
    end
 end
 
@@ -195,7 +198,7 @@ end
 
 function Threads:addjob(...) -- endcallback is passed with returned values of callback
    checkrunning(self)
-   if #self.errors > 0 then self:synchronize() end -- if errors exist, sync immediately.
+   self.errors = false
    local endcallbacks = self.endcallbacks
 
    local idx, threadqueue, r, callback, endcallback
@@ -242,8 +245,9 @@ function Threads:addjob(...) -- endcallback is passed with returned values of ca
 end
 
 function Threads:haserror()
-   checkrunning(self)
-   return (#self.errors > 0)
+   -- DEPRECATED; errors are now propagated immediately
+   -- so the caller doesn't need to explicitly do anything to manage them
+   return false
 end
 
 function Threads:hasjob()
@@ -255,18 +259,14 @@ function Threads:synchronize()
    if not self:isrunning() then
       return
    end
+   self.errors = false
    while self:hasjob()do
       self:dojob()
    end
-   if self:haserror() then
-      local msg = string.format('\n%s', table.concat(self.errors, '\n'))
-      self.errors = {}
-      error(msg)
-   end
 end
 
 function Threads:terminate()
-   if not self:isrunning() then
+   if not self:isrunning() or self.errors then
       return
    end
 
-- 
cgit v1.2.3