Patchwork [4,of,5] chgserver: add an explicit gc to trigger __del__

login
register
mail settings
Submitter Jun Wu
Date March 15, 2016, 9:58 a.m.
Message ID <1dcd622a1f8ffdbfedad.1458035915@x1c>
Download mbox | patch
Permalink /patch/13895/
State Accepted
Commit 5346e9b910fcccef4b1b294d6291a08fda8c3e15
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 15, 2016, 9:58 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1457996883 0
#      Mon Mar 14 23:08:03 2016 +0000
# Node ID 1dcd622a1f8ffdbfedad1a284dc09c5ea409712d
# Parent  c769f8196ccfdfa546862b0c4f608b78ca80a1c7
chgserver: add an explicit gc to trigger __del__

SocketServer.ForkingMixIn uses os._exit which will skip all cleanup handlers.
We want to run __del__ to make things like transactions work.
This patch adds a "gc.collect()" to trigger __del__. It is helpful for chg
to pass some test cases in test-devel-warnings.t.
Yuya Nishihara - March 17, 2016, 2:18 p.m.
On Tue, 15 Mar 2016 09:58:35 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1457996883 0
> #      Mon Mar 14 23:08:03 2016 +0000
> # Node ID 1dcd622a1f8ffdbfedad1a284dc09c5ea409712d
> # Parent  c769f8196ccfdfa546862b0c4f608b78ca80a1c7
> chgserver: add an explicit gc to trigger __del__
> 
> SocketServer.ForkingMixIn uses os._exit which will skip all cleanup handlers.
> We want to run __del__ to make things like transactions work.
> This patch adds a "gc.collect()" to trigger __del__. It is helpful for chg
> to pass some test cases in test-devel-warnings.t.

I think these tests are incompatible with chg by nature because they rely
on gc. Perhaps we should disable them in chg.
Jun Wu - March 18, 2016, 1:25 p.m.
On 03/17/2016 07:18 AM, Yuya Nishihara wrote:
> I think these tests are incompatible with chg by nature because they rely on
> gc. Perhaps we should disable them in chg.

They rely on normal exit, which will call __del__ for every Python objects,
which is fine in my opinion.

"os._exit" (used by SocketServer) is the abnormal way to exit that needs
workaround.
Yuya Nishihara - March 18, 2016, 2:54 p.m.
On Fri, 18 Mar 2016 06:25:12 -0700, Jun Wu wrote:
> On 03/17/2016 07:18 AM, Yuya Nishihara wrote:
> > I think these tests are incompatible with chg by nature because they rely on
> > gc. Perhaps we should disable them in chg.
> 
> They rely on normal exit, which will call __del__ for every Python objects,
> which is fine in my opinion.

Is implicit tr.release() still allowed? I think it should be ditched because
it makes the locking/transaction order unstable.

> "os._exit" (used by SocketServer) is the abnormal way to exit that needs
> workaround.

Well, I agree _exit() isn't normal, but tr.release() shouldn't rely on exit().
Even without chg, you can do "runcommand buggylocking" more than once in the
same process.
Jun Wu - March 18, 2016, 5:59 p.m.
On 03/18/2016 07:54 AM, Yuya Nishihara wrote:
> Well, I agree _exit() isn't normal, but tr.release() shouldn't rely on
> exit(). Even without chg, you can do "runcommand buggylocking" more than
> once in the same process.

I think they are 2 issues. First as chg, not calling __del__ will just surprise
people. Transaction is not the only  __del__ user. There are some other places
like {ssh,http}peer.py, atomictempfile, dirstateguard, etc. Thus this patch is
necessary, as it's hard to touch SocketServer code to change os._exit.

The second step to solve the transaction issue.
Yuya Nishihara - March 18, 2016, 10:28 p.m.
On Fri, 18 Mar 2016 10:59:47 -0700, Jun Wu wrote:
> On 03/18/2016 07:54 AM, Yuya Nishihara wrote:
> > Well, I agree _exit() isn't normal, but tr.release() shouldn't rely on
> > exit(). Even without chg, you can do "runcommand buggylocking" more than
> > once in the same process.
> 
> I think they are 2 issues. First as chg, not calling __del__ will just surprise
> people. Transaction is not the only  __del__ user. There are some other places
> like {ssh,http}peer.py, atomictempfile, dirstateguard, etc. Thus this patch is
> necessary, as it's hard to touch SocketServer code to change os._exit.

I see. Queued this patch, thanks. I've updated the commit message to include
the other uses of __del__.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -45,6 +45,7 @@ 
 
 import SocketServer
 import errno
+import gc
 import inspect
 import os
 import re
@@ -588,6 +589,9 @@ 
                 cerr = commandserver.channeledoutput(self.wfile, 'e')
             traceback.print_exc(file=cerr)
             raise
+        finally:
+            # trigger __del__ since ForkingMixIn uses os._exit
+            gc.collect()
 
 def _reporoot(repo):
     if not repo: