Dp/memory leak fix

[dphilla]

see: #132

[clarkmcc]

I got part way through writing some of these review items before it dawned on me that maybe the separation of the channel and pool implementations were intended to be separated. Or maybe they weren't. In any case I see at least two ways this could be implemented:

1. Based on the maxSize either rely on the sync.Pool or on the channel, but not really both.
2. Always run the sync.Pool and channel modes regardless of the maxSize. You would always pull from channel first and sync.Pool second. When maxSize is 0, you'd virtually never get a value on the channel and always default to the pool -- perfect. If maxSize is > 0, then there's a chance that theres a module waiting for you on the channel, otherwise you get it from the pool.

The advantage I see to option 2 is that it requires less cognitive overhead and its behavior is almost identical to option 1 in practice and the differences are really only theoretical.

[clarkmcc]

Why does new accept a *module[T] as a parameter? Seems like it should only return new modules.

[clarkmcc]

Should be safe to always push to the channel and default to the pool. If the channel is unbuffered (which is the case when maxSize = 0) then you'd automatically drop back to the pool. I personally would always let the pool do the module deallocation when it wants to and never manually call p.close the whole point of the pool (imo) is that allocated modules can be shared and I think there are some wins there even when maxSize > 0.

Both approaches are pretty close to equivalent in practice, this approach (if valid) is probably just simpler to reason about.

Suggested change

	if p.maxSize == 0 {
	p.pool.Put(m)
	} else {
	select {
	case p.ch <- m:
	default:
	// Channel is full, call the close function
	p.close(m)
	}
	}
	select {
	case p.ch <- m:
	default:
	p.pool.Put(m)
	}

[clarkmcc]

Is this necessary if we have runtime finalizers calling m.Close already?

[clarkmcc]

Seems like if the pool is empty, this function returns a nil module. Since the sync.Pool's new property isn't specified, the pool will not automatically create its own module.

[clarkmcc]

If we got to this line, m would have been nil and p.new would likely panic.

[clarkmcc]

An entirely stylistic suggestion that doesn't functionally matter in the slightest.

Suggested change

	for key := range r.activeModules {
	r.activeModules[key].instantiatedModule.CloseWithExitCode(r.config.context, 0)
	delete(r.activeModules, key)
	}
	for key, module := range r.activeModules {
	module.instantiatedModule.CloseWithExitCode(r.config.context, 0)
	delete(r.activeModules, key)
	}

[clarkmcc]

As far as I can tell, calls to modulePool.new actually pass null pointers, so it seems like this should be panicing. This approach seems closer.

Suggested change

	new: func(m *module[T]) (*module[T], error) {
	m.SetFinalizer()
	return newModule[T](ctx, template)
	},
	new: func() (*module[T], error) {
	m := newModule[T](context.Background(), template)
	m.SetFinalizer()
	return m, nil
	},

[clarkmcc]

The ctx being used here is the context provided in calls to newModulePool. I'm not familiar with where the context passed to newModulePool ultimately comes from, but if it comes from the user, the context might be dead by the time this pool wants to allocate new modules in the future.

[clarkmcc]

Here's what this code could look like under the option 2 approach.

Suggested change

	if p.maxSize == 0 {
	m, ok := p.pool.Get().(*module[T])
	if ok && m != nil {
	return m, nil
	}
	return p.new(m)
	}
	// Use buffered channel
	select {
	case m := <-p.ch:
	return m, nil
	default:
	// Channel is empty, create a new module?
	m, _ := p.pool.Get().(*module[T])
	return m, nil
	select {
	case m := <-p.ch:
	return m, nil
	default:
	m, ok := p.pool.Get().(*module[T])
	if ok && m != nil {
	return m, nil
	}
	return p.new()