Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMPROVED] Reuse buffers in Conn.processOpError #341

Merged
merged 1 commit into from
Mar 1, 2018
Merged

[IMPROVED] Reuse buffers in Conn.processOpError #341

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions nats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1475,10 +1475,12 @@ func (nc *Conn) processOpErr(err error) {
nc.conn = nil
}

// Create a new pending buffer to underpin the bufio Writer while
// we are reconnecting.
nc.pending = &bytes.Buffer{}
nc.bw = bufio.NewWriterSize(nc.pending, nc.Opts.ReconnectBufSize)
// Reset pending buffers before reconnecting.
if nc.pending == nil {
nc.pending = new(bytes.Buffer)
Copy link
Member

Choose a reason for hiding this comment

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

This does not account for nc.Opts.ReconnectBufSize, which is worrisome that tests pass, which means that this is not properly tested.
More importantly, in which situation would you get this called so many times and so fast that the GC would not collect those buffers? Maybe we need to understand better why this is a problem in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kozlovic thanks for the feedback!

The tests likely passed because bufio.NewWriterSize(W, N) does nothing to limit the amount of data written io.Writer W - N here is only used to size the internal buffer. Properly limiting the amount of data writted to W would need to be handled by W - implementing a LimitedWriter like io.LimitedReader and wrapping the buffer passed to nc.pending would work - this would likely require updating the error handling around reconnects.

The reason this was spinning so hard and generating loads of garbage was that we use ginkgo for our tests and it encourages some nasty interesting patterns - like spinning up a nats listener for each test irregardless of whether it has something to connect to - it is spun down at the end of each test, but who knows what the connection state is then. Because of the nastiness ginkgo introduces it does tend to highlight situations where applications do not perform well when error'ing.

That said, this is an edge case that occurs only when error'ing in a tight loop in test code, but an 8Mb memory allocation per reconnect is still a pretty heavy hit (regardless of the efficacy of Go's GC).

Thanks again for reviewing and please let me know if there is anything I may do to help.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the underlying bytes buffer is never limiting the amount of data that can be written. We actually do check for length in the publish() call to enforce that limit (this is to avoid unbound growth).
Let me investigate a bit more to see why the original code would need so much memory, but otherwise I think your changes are good. Thanks!

}
nc.pending.Reset()
nc.bw.Reset(nc.pending)

go nc.doReconnect()
nc.mu.Unlock()
Expand Down