increase UDP receive buffer size #2791
Conversation
| } | ||
| size, err := inspectReadBuffer(c) | ||
| if err != nil { | ||
| log.Printf("Failed to determine receive buffer size: %s", err) |
There was a problem hiding this comment.
I'm using log here, so that users will actually see the log message (even if QUIC_GO_LOG_LEVEL is not set).
There was a problem hiding this comment.
Why would this always need to log? What are the failure modes here that require user action?
We support custom pconns, which don't need to implement SyscallConn(), and could benignly trigger this?
There was a problem hiding this comment.
We do, but I'm assuming that all pconns at some point wrap a UDP conn. QUIC v1 is only defined to work over UDP anyway.
Codecov Report
@@ Coverage Diff @@
## master #2791 +/- ##
==========================================
- Coverage 85.96% 85.83% -0.14%
==========================================
Files 133 134 +1
Lines 8990 9030 +40
==========================================
+ Hits 7728 7750 +22
- Misses 928 939 +11
- Partials 334 341 +7
Continue to review full report at Codecov.
|
| @@ -2,6 +2,9 @@ package protocol | |||
|
|
|||
| import "time" | |||
|
|
|||
| // DesiredReceiveBufferSize is the kernel UDP receive buffer size that we'd like to use. | |||
| const DesiredReceiveBufferSize = (1 << 20) * 2 // 2 MB | |||
There was a problem hiding this comment.
Not sure if this is what we really need, we might need to adjust this constant after more extensive benchmarking.
| } | ||
| size, err := inspectReadBuffer(c) | ||
| if err != nil { | ||
| log.Printf("Failed to determine receive buffer size: %s", err) |
There was a problem hiding this comment.
Why would this always need to log? What are the failure modes here that require user action?
We support custom pconns, which don't need to implement SyscallConn(), and could benignly trigger this?
marten-seemann
force-pushed
the
udp-receive-buffer-size
branch
from
46f7642
to
a46c9ca
Compare
marten-seemann
force-pushed
the
udp-receive-buffer-size
branch
from
a46c9ca
to
14a5aa8
Compare
|
Merging this PR for now. We can still change the log messages later, if they turn out to be too annoying. |
| @@ -7,3 +7,7 @@ import "net" | |||
| func newConn(c net.PacketConn) (connection, error) { | |||
| return &basicConn{PacketConn: c}, nil | |||
| } | |||
|
|
|||
| func inspectReadBuffer(net.PacketConn) (int, error) { | |||
| return 0, nil | |||
There was a problem hiding this comment.
Any particular reason why golang.org/x/sys wasn't used? GetsockoptInt should work for Windows in that package.
There was a problem hiding this comment.
@dswarbrick Not sure I understand. Can you elaborate?
There was a problem hiding this comment.
It appears that the inspectReadBuffer function here is a no-op on Windows, presumably because syscall.GetsockoptInt in the standard library is not implemented for Windows:
func GetsockoptInt(fd Handle, level, opt int) (int, error) { return -1, EWINDOWS }Since the syscall package is frozen, that functionality is not likely to make it into the standard library. However, it is implemented in the golang.org/x/sys/windows package, as described in the link I posted.
There was a problem hiding this comment.
@dswarbrick Thanks for pointing this out, I wasn't aware of the syscall package being frozen.
Would you mind sending us a PR that adds this functionality on Windows?
Fixes #2255. Ref #2586.
... and emit a log message if we can't do so.