Skip to content

Conversation

@jzbyers
Copy link
Contributor

@jzbyers jzbyers commented Oct 10, 2018

Bug Description

When creating a stream via CreateStream(), a new stream is always created even if a stream with the same id already exists, causing a memory leak where goroutines are created via str.run() but never closed.

How to Read This PR

This PR fixes that bug by closing the newly created stream if the stream id is found in the server's map of streams.

How to Test This PR

These changes are reflected in the tests, but there is a potential race condition where the newly created stream might not have been closed by the time the test checks whether runtime.NumGoroutine() is equal to numGoRoutines.

At the moment I can't think of a better, simple way to test this so if you'd prefer to not have an imperfect test in server_test.go, let me know and I will remove it. Thanks!

make test

server.go Outdated
defer s.mu.Unlock()

if s.Streams[id] != nil {
str.close()
Copy link
Member

@purehyperbole purehyperbole Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I think this can be avoided altogether, by reordering this function so that we create a stream after checking if it already exists. Something like:

func (s *Server) CreateStream(id string) *Stream {
	s.mu.Lock()
	defer s.mu.Unlock()

	if s.Streams[id] != nil {
		return s.Streams[id]
	}

	str := newStream(s.BufferSize, s.AutoReplay)
	str.run()

	s.Streams[id] = str

	return str
}

That should avoid the race condition you mentioned. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! That's simpler to understand. Changing now.

@jzbyers jzbyers closed this Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants