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.

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

Refactor UDP & TCP input buffers #992

Closed
wants to merge 0 commits into from
Closed

Refactor UDP & TCP input buffers #992

wants to merge 0 commits into from

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Apr 7, 2016

closes #991

this is a WIP, and requires some testing, also need to port the udp buffer fix to the statsd server

@mb-m
Copy link

mb-m commented Apr 7, 2016

I think you still might be better with the bufio.Scanner, as you don't know when Read() might return off a socket. The bufio.Scanner allows you to Read() until you properly get EOF, while handing you lines.

@mb-m
Copy link

mb-m commented Apr 7, 2016

I should point out that I don't think the same issues apply to the UDP listener, as to use UDP you have an unconnected socket, and you use a sendto() which should tell you what size packet you're sending. On the TCP listener, though, what the caller does with write() or a send() does not necessarily correspond with the read() / recv() - which is essentially the same race as #991 points out between the internal threads. This means that your parser is almost guaranteed a malformed packet if the data is longer than the page size, because you'll likely break at a non-useful point.

@sparrc
Copy link
Contributor Author

sparrc commented Apr 7, 2016

You're right about using bufio.Scanner, I'm going to keep that portion but basically use what you suggested, creating a copy of the length of the buffer at each loop.

I'm not sure I follow what you mean about the UDP listener though. If I am creating the []byte buffer outside the for-loop, I'm fairly certain there is a chance of a data race unless I allocate and copy the buffer within the loop.

@sparrc sparrc force-pushed the udp-tcp-buf branch 3 times, most recently from 7c2051e to db71e25 Compare April 7, 2016 22:16
@@ -283,9 +285,11 @@ func (s *Statsd) udpListen() error {
log.Printf("ERROR READ: %s\n", err.Error())
continue
}
bufCopy := make([]byte, n)
Copy link
Contributor

Choose a reason for hiding this comment

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

If people are hitting this listener at a high enough rate to expose the previous race condition, you might want to prefer a sync.Pool of byte slices to reduce allocations and garbage. But, I'm just speculating, and writing a benchmark will be more informative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't thought about using a pool, I'll have to look into that.

I'm not totally clear though how the pool handles sized slices, since that's the important bit of this line. Would I have to get from the pool and then resize each time? That seems like it would be less efficient than leaving it to the compiler but I could try to write up some benchmarking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could be reading this wrong, but these comments make me think this isn't the correct use-case of a Pool (from https://golang.org/pkg/sync/#Pool):

An appropriate use of a Pool is to manage a group of temporary items silently 
shared among and potentially reused by concurrent independent clients of a 
package. Pool provides a way to amortize allocation overhead across many 
clients.

 ...

On the other hand, a free list maintained as part of a short-lived object is not a 
suitable use for a Pool, since the overhead does not amortize well in that scenario. 
It is more efficient to have such objects implement their own free list.

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.

telegraf tcp_listener can corrupt data
3 participants