deadlock on _sendlock caused by GC on netref

[amirmiron]

No description provided.

[ericfrederich]

This commit just messed me up. Jython's gc.disable raises a NotImplementedError.
I'd hate for rpyc to have to do a runtime check for Jython.
Are we sure this is the best way to solve the issue?

[tomerfiliba]

@amirmiron - any thoughts?

[ericfrederich]

@amirmiron @tomerfiliba I think it is really important that rpyc work on as many Python implementations as possible since it is implemented in pure Python (along with its dependenceis). A great use case for rpyc in addition to talking to python on other machines is talking to different Python implementations on the same machine.

[amirmiron]

@tomerfiliba @ericfrederich Well we can change __del__ on netrefs to not call asyncreq(HANDLE_DEL) but just add this to some queue(well not "this", as it's being collected, but it's oid and conn object), which has a bg thread(started on connect) monitoring it and calling the asyncreqs.
Should allow it to work on all implementations, but requires adding another thread on connection creation. Can't think of any other solution we can have to this though as the called __del__ is not in our control. What do you think?

[ericfrederich]

I wanted to see if there was a Java equivalent of gc.disable so I stopped by #java on freenode.
Here is some of our discussion if it helps any...

> <ELFrederich>    is there a way to programatically disable garbage collection? 
> <ojacobson>      System.exit()
> <ojacobson>      GC _may_ cease if you stop allocating, but even that's not guaranteed (not to mention very hard to do)
> <ELFrederich>    ojacobson, this is why I'm asking.  This commit broke Jython's ability to use this package because their solution for a deadlock was to temporarily disable garbage collection.  https://github.com/amirmiron/rpyc/commit/8c1eff86d14da8982fb292494ff613a14e3bae9e
> <ojacobson>      ELFrederich: the equivalent feature in Java would be to use various kinds of weak reference
> <ojacobson>      though honestly any "fix" that relies on stopping GC points to serious bugs elsewhere; I doubt that is a good fix
> <ojacobson>      objects in use should be strongly reachable at all times
> <ojacobson>      at which point no GC -- not even CPython's -- will touch them
> <ELFrederich>    ojacobson, my thoughts exactly.  I ran into the bug after it was merged and went back and commented on it.  I asked "Are we sure this is the best way to solve the issue?"
> <ELFrederich>    https://github.com/tomerfiliba/rpyc/pull/166
> <ojacobson>      also python code that overrides __del__ should be treated as buggy by default
> <ojacobson>      same as Java programs that override finalize()
> <ojacobson>      The sane idiom in Python for scoped lifetimes (what people usually want __del__ for) is `with` blocks; in Java, `try() {}` blocks.
> <ELFrederich>    ojacobson, can you look at the last comment on that pull request.  The one mentioning adding a queue
> <freeone3000>    I don't see how that's a deadlock - I see two threads after the same lock, yes, but that's.. not deadlocky.
> <freeone3000>    They both free in the finally, so what would happen, at worst, is that you send data during garbage collection, and the gc thread is locked until a network send completes (two, if you count the one in __del)
> <freeone3000>    Maybe I don't understand enough python to see the problem with that - it wouldn't be a problem in Java, it'd just be inefficient and questionable code.
> <ojacobson>      They're getting screwed by __del__
> <ELFrederich>    freeone3000, I'm not exactly sure either.  It says BaseNetRef.__del__ must call asyncreq.  I doesn't say, but I'm assuming this also calls _send somehow?
> <ojacobson>      whose python rules, both formal and practical, are much weirder than Java's .finalize() rules
> <ojacobson>      The bug is "you have a __del__ method"
> <freeone3000>    ELFrederich: Yes. In Java, this wouldn't be a deadlock, you'd just have the GC finalizer thread in a WAIT state until send() completes.
> <freeone3000>    ELFrederich: Which is a problem for a ton of reasons, but deadlock won't be one of them.
> <freeone3000>    ELFrederich: Of course, you'd actually do it in a try-with-resources block, so what would happen is the thread that created the object would actually be in a WAIT state until all send()s complete, which is downright reasonable.
> <ELFrederich>    freeone3000, ojacobson you guys mind if I copy / paste this discussion into the GitHub thread?
> <freeone3000>    Sure, why not.
> <ojacobson>      They're going to argue that the __del__ method is a backstop against connection leaks, and that's a sane rationale, but the times at which __del__ gets run are hostile
> <ojacobson>      it should detect, not intervene, or just plain go away
> <ojacobson>      I see that the affected class is also a context manager, which is the right way to handle this

[amirmiron]

Well ojacobson says the same thing, that __del__ will only detect, i.e. notice that it should be deleted and add to some list, which is what I suggested, but it has its overheads, I still want to know if Tomer is okay with adding another bgthread for it.

[ericfrederich]

Since this is a closed request I'll start a new one about Jython not working.