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

Buffer for reading udp packets is too small. #35

Closed
TursMA opened this issue Apr 29, 2016 · 5 comments
Closed

Buffer for reading udp packets is too small. #35

TursMA opened this issue Apr 29, 2016 · 5 comments

Comments

@TursMA
Copy link

TursMA commented Apr 29, 2016

512 bytes are ok for the wild Internet. For local network we can send udp packets up to 1432 bytes or greater. For example it's very important if we use buffered client (github.com/cactus/go-statsd-client/statsd/client_buffered.go). So I suggest increasing buf size to 2-8k.
Also MTU is important for client. Udp packet can be split or dropped by ISP because of size. Server shouldn't evaluate MTU.

{code}
func (l *StatsDListener) Listen(e chan<- Events) {
// TODO: evaluate proper size according to MTU
var buf [512]byte
for {
n, _, err := l.conn.ReadFromUDP(buf[0:])
if err != nil {
log.Fatal(err)
}
l.handlePacket(buf[0:n], e)
}
}
{code}

@juliusv
Copy link
Member

juliusv commented Apr 29, 2016

You're right, that should definitely be increased and the comment doesn't make too much sense. No idea why it was that low in the first place!

See #36

@TursMA
Copy link
Author

TursMA commented Apr 29, 2016

Thank you very much!

@TursMA TursMA closed this as completed Apr 29, 2016
@SuperQ
Copy link
Member

SuperQ commented Apr 29, 2016

The docs default the max size to 512b0 for "open internet" safety concerns. But this can be configured by the admin.

This may be something we want to have as a runtime flag if possible.

@TursMA TursMA reopened this Apr 29, 2016
@TursMA
Copy link
Author

TursMA commented Apr 29, 2016

Flag is overhead, I think.
Max UPD packet size is (65,535 − 8 byte UDP header − 20 byte IP header)
For message-based sockets, such as SOCK_DGRAM and SOCK_SEQPACKET, the entire message shall be read in a single operation.
So buffer[65,535] covers all possible cases. We allocate it just once, so fix is changing buffer to max allowable size. =)

@SuperQ
Copy link
Member

SuperQ commented Apr 29, 2016

Yea, that seems reasonable.

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

No branches or pull requests

3 participants