[FIXED] Reduce in-use memory space on connection close

[kozlovic]

Setting internal subscriptions and subMap to nil on subscription
close seem to eliminate the inuse_space report from go tool pprof

Resolves #251

Signed-off-by: Ivan Kozlovic ivan@synadia.com

[coveralls]

Coverage increased (+0.03%) to 93.793% when pulling 1151bee on fix_251 into 3e2ff07 on master.

[derekcollison]

LGTM

[JensRantil]

🤔 So, I'm curious to hear what the actual reason for this not getting garbage collected was. In my test example, I had no reference to nc, nor sc, including no goroutines running that could hold a reference to any of these. Did you figure this out?

[JensRantil]

More of a thought, I just learnt that finalizers don't run on circular references, but I assume https://github.com/nats-io/go-nats-streaming/blob/3e2ff0719c7a6219b4e791e19c782de98c701f4a/stan.go#L452 has nothing to do with this?

[kozlovic]

@JensRantil I could swear that this is the first thing I tried (commenting out the call to the finalizer), but I may have done something wrong because I could still see the issue.
My guess is that I was mistakenly looking at alloc_space (which is the default for me when running go tool pprof ./<file>) instead of inuse_space.

I then tried looking a the subs that were not unsubscribed on Close() when the streaming library creates the NATS connection - since we don't need because we close the NATS connection - and by setting them to nil the inuse_space went down to "zero".

Anyway, with the test that I added and your test, removing the call to SetFinalizer() and looking at inuse_space, I can see that the problem is solved.

The NATS Subscriptions have a reference to the NATS connection, and I think that without them being set to nil it would prevent the finalizer from running but would cause a reference to be held. Setting the subscriptions to nil I can see that the finalizer runs. Without setting them to nil, it does not run.

But again, simply removing the finalizer works. So the question now is that: is the finalizer worth it? If not, I would gladly revert the changes and simply remove the call to finalizer. If we think it is worth having it, then the current fix is correct. @derekcollison, wdyt?

[JensRantil]

I vote for removing the finalizer instead of breaking the circular reference. I've rarely seen a finalizer being needed and I'm not sure it's needed here since I suppose open files and sockets all have a finalizer on them to be closed anyway.