liuxfiu / simulus

Simulus - A Discrete-Event Simulator in Python
MIT License
9 stars 2 forks source link

_Process instances leaked #22

Open curob opened 1 year ago

curob commented 1 year ago

Problem

_Process instances created via the simulator.process() method are never released and therefore they (and their contained greenlet instance are leaked). This can be a big issue for long running simulations that dynamically create processes. This is also means that any local variables in the process methods will also be leaked.

Reproducer

from simulus import simulator

sim = simulator()

def thread():
    sim.sleep(5)

procs = [sim.process(thread) for _ in range(0, 10)]

# Run all processes to completion
sim.run()
assert sim.now == 5

# Printing the greenlet will indicate "... suspended active started"
print(procs[0].vert)

# Now lets look for memory leaks...
import gc
del procs
gc.collect()
objs = gc.get_objects()
# Grab the first leaked _Process object and see who is holding onto it...
p = next(filter(lambda t: t.__class__.__name__ == "_Process", objs))
refs = gc.get_referrers(p)

# One of the referrers will be: <bound method _Process.invoke> (this is the method that is passed to the greenlet constructor)

Based on the above, I believe that the greenlet associated with the _Process is never fully being cleaned up and therefore its reference to the owning _Process (via the invoke method) is never being released and therefore the garbage collector cannot free the _Process.

Cause

After examining the _Process code, it appears that the problem is the _Process.terminate() method is never allowed to return. Instead, it explicitly calls self.main.switch. This means that the contained greenlet's reference to the _Process.invoke method will never complete (because the code always switches to another greenlet right before it completes) and therefore the greenlet infrastructure doesn't know that it can cleanup the thread because technically it never finishes.

Proposed Solution:

In _Process.terminate change:

self.main.switch()

to:

self.vert.parent = self.main

Greenlet will automatically switch to a dead thread's parent ("When a greenlet dies, in which case execution jumps to the parent greenlet." ref: https://greenlet.readthedocs.io/en/latest/switching.html).

Proof

I made the above change locally and reran my reproducer. Now when I examine the vert instance of a completed process it is reported as "dead". When I run the garbage collection steps in the reproducer, no instances of _Process are found.

curob commented 1 year ago

After looking at this some more, I do not think the proposed fix is enough as it fixes the memory leak but appears to cause another problem: _Process instances cannot be killed via the calls to terminate() that are not a result of the greenlets invoke method ending.

E.g. if another process calls simulator.kill(process) or (while running) a process calls simulator.kill() (in order to kill itself) then I do not think this will work anymore.

At this time, my code does not rely on killing running processes so this is not an issue for me (at the moment). If I find a full fix then I will post it here.