Multiple errors in '__toclose' report the first one

roberto-ieru · roberto-ieru · commit b4d5dff8ec4f · 2019-06-05T13:16:25.000-03:00
When there are multiple errors when closing objects, the error
reported by the protected call is the first one, for two reasons:
First, other errors may be caused by this one;
second, the first error is handled in the original execution context,
and therefore has the full traceback.
diff --git a/lcorolib.c b/lcorolib.c
@@ -75,11 +75,8 @@ static int luaB_auxwrap (lua_State *L) {
   int r = auxresume(L, co, lua_gettop(L));
   if (r < 0) {
     int stat = lua_status(co);
-    if (stat != LUA_OK && stat != LUA_YIELD) {
-      stat = lua_resetthread(co);  /* close variables in case of errors */
-      if (stat != LUA_OK)  /* error closing variables? */
-        lua_xmove(co, L, 1);  /* get new error object */
-    }
+    if (stat != LUA_OK && stat != LUA_YIELD)
+      lua_resetthread(co);  /* close variables in case of errors */
     if (lua_type(L, -1) == LUA_TSTRING) {  /* error object is a string? */
       luaL_where(L, 1);  /* add extra info, if available */
       lua_insert(L, -2);
diff --git a/lfunc.c b/lfunc.c
@@ -144,13 +144,16 @@ static int callclosemth (lua_State *L, TValue *uv, StkId level, int status) {
       luaG_runerror(L, "attempt to close non-closable variable '%s'", vname);
     }
   }
-  else {  /* there was an error */
+  else {  /* must close the object in protected mode */
+    ptrdiff_t oldtop = savestack(L, level + 1);
     /* save error message and set stack top to 'level + 1' */
     luaD_seterrorobj(L, status, level);
     if (prepclosingmethod(L, uv, s2v(level))) {  /* something to call? */
-      int newstatus = luaD_pcall(L, callclose, NULL, savestack(L, level), 0);
-      if (newstatus != LUA_OK)  /* another error when closing? */
+      int newstatus = luaD_pcall(L, callclose, NULL, oldtop, 0);
+      if (newstatus != LUA_OK && status == CLOSEPROTECT)  /* first error? */
         status = newstatus;  /* this will be the new error */
+      else  /* leave original error (or nil) on top */
+        L->top = restorestack(L, oldtop);
     }
     /* else no metamethod; ignore this case and keep original error */
   }
diff --git a/manual/manual.of b/manual/manual.of
@@ -1541,23 +1541,30 @@ if there was no error, the second argument is @nil.
 
 If several to-be-closed variables go out of scope at the same event,
 they are closed in the reverse order that they were declared.
+
 If there is any error while running a closing method,
 that error is handled like an error in the regular code
 where the variable was defined;
 in particular,
 the other pending closing methods will still be called.
+After an error,
+other errors in closing methods
+interrupt the respective method,
+but are otherwise ignored;
+the error reported is the original one.
 
 If a coroutine yields inside a block and is never resumed again,
 the variables visible at that block will never go out of scope,
 and therefore they will never be closed.
 Similarly, if a coroutine ends with an error,
 it does not unwind its stack,
 so it does not close any variable.
-You should either use finalizers
-or call @Lid{coroutine.close} to close the variables in these cases.
-However, note that if the coroutine was created
+In both cases,
+you should either use finalizers
+or call @Lid{coroutine.close} to close the variables.
+However, if the coroutine was created
 through @Lid{coroutine.wrap},
-then its corresponding function will close all variables
+then its corresponding function will close the coroutine
 in case of errors.
 
 }
@@ -3932,7 +3939,7 @@ Returns a status code:
 @Lid{LUA_OK} for no errors in closing methods,
 or an error status otherwise.
 In case of error,
-leave the error object on the stack,
+leaves the error object on the top of the stack,
 
 }
 
@@ -6355,6 +6362,7 @@ Closes coroutine @id{co},
 that is,
 closes all its pending to-be-closed variables
 and puts the coroutine in a dead state.
+The given coroutine must be dead or suspended.
 In case of error closing some variable,
 returns @false plus the error object;
 otherwise returns @true.
@@ -6412,7 +6420,8 @@ true when the running coroutine is the main one.
 
 Returns the status of the coroutine @id{co}, as a string:
 @T{"running"},
-if the coroutine is running (that is, it called @id{status});
+if the coroutine is running
+(that is, it is the one that called @id{status});
 @T{"suspended"}, if the coroutine is suspended in a call to @id{yield},
 or if it has not started running yet;
 @T{"normal"} if the coroutine is active but not running
diff --git a/testes/coroutine.lua b/testes/coroutine.lua
@@ -163,15 +163,23 @@ do
   assert(not X and coroutine.status(co) == "dead")
 
   -- error closing a coroutine
+  local x = 0
   co = coroutine.create(function()
+    local <toclose> y = func2close(function (self,err)
+      if (err ~= 111) then os.exit(false) end   -- should not happen
+      x = 200
+      error(200)
+    end)
     local <toclose> x = func2close(function (self, err)
       assert(err == nil); error(111)
     end)
     coroutine.yield()
   end)
   coroutine.resume(co)
+  assert(x == 0)
   local st, msg = coroutine.close(co)
-  assert(not st and coroutine.status(co) == "dead" and msg == 111)
+  assert(st == false and coroutine.status(co) == "dead" and msg == 111)
+  assert(x == 200)
 
 end
 
diff --git a/testes/locals.lua b/testes/locals.lua
@@ -267,14 +267,14 @@ do   -- errors in __close
     if err then error(4) end
   end
   local stat, msg = pcall(foo, false)
-  assert(msg == 1)
-  assert(log[1] == 10 and log[2] == 3 and log[3] == 2 and log[4] == 2
+  assert(msg == 3)
+  assert(log[1] == 10 and log[2] == 3 and log[3] == 3 and log[4] == 3
          and #log == 4)
 
   log = {}
   local stat, msg = pcall(foo, true)
-  assert(msg == 1)
-  assert(log[1] == 4 and log[2] == 3 and log[3] == 2 and log[4] == 2
+  assert(msg == 4)
+  assert(log[1] == 4 and log[2] == 4 and log[3] == 4 and log[4] == 4
          and #log == 4)
 
   -- error in toclose in vararg function
@@ -317,15 +317,15 @@ if rawget(_G, "T") then
     local <toclose> x = setmetatable({}, {__close = function ()
       T.alloccount(0); local x = {}   -- force a memory error
     end})
-    error("a")   -- common error inside the function's body
+    error(1000)   -- common error inside the function's body
   end
 
   stack(5)    -- ensure a minimal number of CI structures
 
   -- despite memory error, 'y' will be executed and
   -- memory limit will be lifted
   local _, msg = pcall(foo)
-  assert(msg == "not enough memory")
+  assert(msg == 1000)
 
   local close = func2close(function (self, msg)
     T.alloccount()
@@ -368,8 +368,7 @@ if rawget(_G, "T") then
   end
 
   local _, msg = pcall(test)
-  assert(msg == 1000)
-
+  assert(msg == "not enough memory")   -- reported error is the first one
 
   do    -- testing 'toclose' in C string buffer
     collectgarbage()
@@ -453,15 +452,27 @@ end
 
 do
   -- error in a wrapped coroutine raising errors when closing a variable
-  local x = false
+  local x = 0
   local co = coroutine.wrap(function ()
-    local <toclose> xv = func2close(function () error("XXX") end)
+    local <toclose> xx = func2close(function () x = x + 1; error("YYY") end)
+    local <toclose> xv = func2close(function () x = x + 1; error("XXX") end)
       coroutine.yield(100)
       error(200)
   end)
-  assert(co() == 100)
-  local st, msg = pcall(co)
-  -- should get last error raised
+  assert(co() == 100); assert(x == 0)
+  local st, msg = pcall(co); assert(x == 2)
+  assert(not st and msg == 200)   -- should get first error raised
+
+  x = 0
+  co = coroutine.wrap(function ()
+    local <toclose> xx = func2close(function () x = x + 1; error("YYY") end)
+    local <toclose> xv = func2close(function () x = x + 1; error("XXX") end)
+      coroutine.yield(100)
+      return 200
+  end)
+  assert(co() == 100); assert(x == 0)
+  local st, msg = pcall(co); assert(x == 2)
+  -- should get first error raised
   assert(not st and string.find(msg, "%w+%.%w+:%d+: XXX"))
 end
 

Original file line number	Diff line number	Diff line change
`@@ -144,13 +144,16 @@ static int callclosemth (lua_State L, TValue uv, StkId level, int status) {`
`144`	`144`	`luaG_runerror(L, "attempt to close non-closable variable '%s'", vname);`
`145`	`145`	`}`
`146`	`146`	`}`
`147`		`- else { /* there was an error */`
	`147`	`+ else { /* must close the object in protected mode */`
	`148`	`+ ptrdiff_t oldtop = savestack(L, level + 1);`
`148`	`149`	`/* save error message and set stack top to 'level + 1' */`
`149`	`150`	`luaD_seterrorobj(L, status, level);`
`150`	`151`	`if (prepclosingmethod(L, uv, s2v(level))) { /* something to call? */`
`151`		`- int newstatus = luaD_pcall(L, callclose, NULL, savestack(L, level), 0);`
`152`		`- if (newstatus != LUA_OK) /* another error when closing? */`
	`152`	`+ int newstatus = luaD_pcall(L, callclose, NULL, oldtop, 0);`
	`153`	`+ if (newstatus != LUA_OK && status == CLOSEPROTECT) /* first error? */`
`153`	`154`	`status = newstatus; /* this will be the new error */`
	`155`	`+ else /* leave original error (or nil) on top */`
	`156`	`+ L->top = restorestack(L, oldtop);`
`154`	`157`	`}`
`155`	`158`	`/* else no metamethod; ignore this case and keep original error */`
`156`	`159`	`}`