Disable .step by default to fix memory leak #2

Merged
merged 5 commits into from Jul 17, 2011

Conversation

Projects
None yet
2 participants
@justmoon
Contributor

justmoon commented Jun 30, 2011

By default, Chainsaw retains a history of all actions including all arguments. In some cases where Chainsaw objects are long-lived, such as node-binary, this causes a memory leak.

This patch changes Chainsaw such that no history of actions is kept by default. Instead, the history is enabled automatically as soon as the user calls a function (such as trap, down or jump) that requires it.

This patch should maintain downwards compatibility and should benefit most code using chainsaw without any changes. Tested with node-binary.

Note that two unit tests fail - those were also failing before this patch.

@substack

This comment has been minimized.

Show comment
Hide comment
@substack

substack Jul 11, 2011

Owner

I can't confirm the purported test behavior:

substack : node-chainsaw $ expresso

   100% 9 tests

substack : node-chainsaw $ git checkout justmoon.memory
Switched to branch 'justmoon.memory'
substack : node-chainsaw $ expresso

   uncaught: AssertionError: [8] deepEqual [8,9,-1,8]
    at EventEmitter.<anonymous> (/home/substack/projects/node-chainsaw/test/chainsaw.js:406:20)
    at EventEmitter.emit (events.js:61:17)
    at EventEmitter.next (/home/substack/projects/node-chainsaw/index.js:49:17)
    at Object.y (/home/substack/projects/node-chainsaw/test/chainsaw.js:401:22)
    at EventEmitter.next (/home/substack/projects/node-chainsaw/index.js:54:18)
    at EventEmitter.jump (/home/substack/projects/node-chainsaw/index.js:124:13)
    at Object.y (/home/substack/projects/node-chainsaw/test/chainsaw.js:400:28)
    at EventEmitter.next (/home/substack/projects/node-chainsaw/index.js:54:18)
    at EventEmitter.jump (/home/substack/projects/node-chainsaw/index.js:124:13)
    at Object.y (/home/substack/projects/node-chainsaw/test/chainsaw.js:400:28)


   Failures: 1
Owner

substack commented Jul 11, 2011

I can't confirm the purported test behavior:

substack : node-chainsaw $ expresso

   100% 9 tests

substack : node-chainsaw $ git checkout justmoon.memory
Switched to branch 'justmoon.memory'
substack : node-chainsaw $ expresso

   uncaught: AssertionError: [8] deepEqual [8,9,-1,8]
    at EventEmitter.<anonymous> (/home/substack/projects/node-chainsaw/test/chainsaw.js:406:20)
    at EventEmitter.emit (events.js:61:17)
    at EventEmitter.next (/home/substack/projects/node-chainsaw/index.js:49:17)
    at Object.y (/home/substack/projects/node-chainsaw/test/chainsaw.js:401:22)
    at EventEmitter.next (/home/substack/projects/node-chainsaw/index.js:54:18)
    at EventEmitter.jump (/home/substack/projects/node-chainsaw/index.js:124:13)
    at Object.y (/home/substack/projects/node-chainsaw/test/chainsaw.js:400:28)
    at EventEmitter.next (/home/substack/projects/node-chainsaw/index.js:54:18)
    at EventEmitter.jump (/home/substack/projects/node-chainsaw/index.js:124:13)
    at Object.y (/home/substack/projects/node-chainsaw/test/chainsaw.js:400:28)


   Failures: 1
@justmoon

This comment has been minimized.

Show comment
Hide comment
@justmoon

justmoon Jul 11, 2011

Contributor

Thanks, replicated the issue, I'm working on a fix!

Contributor

justmoon commented Jul 11, 2011

Thanks, replicated the issue, I'm working on a fix!

@justmoon

This comment has been minimized.

Show comment
Hide comment
@justmoon

justmoon Jul 11, 2011

Contributor

Ok, what's happening is that my little automatic upgrade trick cannot work retroactively. node-chainsaw no longer keeps a history of actions by default, so when the jump() call comes, we've already forgotten about the first call to x().

This poses a bit of a dilemma: I still think we must get rid of the history of actions for any use case where a Chainsaw may be long-lived (such as node-binary). At the same time we still want to support .jump() for those that use it. But because we can't see into the future, I don't think we can decide that purely with behind-the-scenes magic now.

Suggestion 1: We can make the upgrade mechanism explicit. If one wishes to use .jump(), one must first call (a new function) .record() to start recording actions. That requires libraries using .jump() to upgrade, but it fixes the memory issue for all others transparently.

Suggestion 2: We can keep the default behavior and add a new subconstructor Chainsaw.light(). Any module that doesn't use jump() (like node-binary) could switch to the new constructor to get a version of Chainsaw that does not retain a history.

With option no 1 there is no "trap" for new users of Chainsaw to accidentally leak memory.
With option no 2 we maintain downwards compatibility with existing libraries that use jump().

Thoughts?

Contributor

justmoon commented Jul 11, 2011

Ok, what's happening is that my little automatic upgrade trick cannot work retroactively. node-chainsaw no longer keeps a history of actions by default, so when the jump() call comes, we've already forgotten about the first call to x().

This poses a bit of a dilemma: I still think we must get rid of the history of actions for any use case where a Chainsaw may be long-lived (such as node-binary). At the same time we still want to support .jump() for those that use it. But because we can't see into the future, I don't think we can decide that purely with behind-the-scenes magic now.

Suggestion 1: We can make the upgrade mechanism explicit. If one wishes to use .jump(), one must first call (a new function) .record() to start recording actions. That requires libraries using .jump() to upgrade, but it fixes the memory issue for all others transparently.

Suggestion 2: We can keep the default behavior and add a new subconstructor Chainsaw.light(). Any module that doesn't use jump() (like node-binary) could switch to the new constructor to get a version of Chainsaw that does not retain a history.

With option no 1 there is no "trap" for new users of Chainsaw to accidentally leak memory.
With option no 2 we maintain downwards compatibility with existing libraries that use jump().

Thoughts?

@justmoon

This comment has been minimized.

Show comment
Hide comment
@justmoon

justmoon Jul 13, 2011

Contributor

Ok, I went with option 2. All tests passing. Maintains downward compatibility.

Contributor

justmoon commented Jul 13, 2011

Ok, I went with option 2. All tests passing. Maintains downward compatibility.

@substack substack merged commit 415d338 into substack:master Jul 17, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment