-
Notifications
You must be signed in to change notification settings - Fork 55
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
Allow for a connection to check if it's sending bytes #324
base: main
Are you sure you want to change the base?
Conversation
x/sockopt/is_sending_bytes_linux.go
Outdated
// 1 == TCP_ESTABLISHED, but for some reason not available in the package | ||
if tcpInfo.State != 1 { | ||
// If the connection is not established, the socket is not sending bytes | ||
return false, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you confirm this does what we supposed the bye-dpi code does? I'm starting having some doubts looking at the list of states
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x/sockopt/sockopt.go
Outdated
return isConnectionSendingBytesImplemented() | ||
} | ||
|
||
func (o *tcpOptions) WaitUntilBytesAreSent() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality is too high-level to live here.
I'd say we just need the bytes not sent and state options.
Then you can implement the logic to wait completely separate.
x/sockopt/is_sending_bytes_linux.go
Outdated
} | ||
|
||
// 1 == TCP_ESTABLISHED, but for some reason not available in the package | ||
if tcpInfo.State != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this relevant? Won't tcpInfo.Notsent_bytes
be zero if the connection is not established?
Also, we won't have the conenction object if it's not established.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this relevant? Won't tcpInfo.Notsent_bytes be zero if the connection is not established?
I don't get this comment. This check is not about connection establishment
Also, we won't have the conenction object if it's not established.
This function should not be called when the connection is not yet established.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying we can remove the State
test. It's not doing anything. tcpInfo.Notsent_bytes
will be zero if it's not established.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what about the cases when TCP connection is closing? This way we can abort waiting loop earlier. Maybe that's why it was there in the original code.
x/sockopt/sockopt.go
Outdated
@@ -50,15 +60,51 @@ var _ HasHopLimit = (*hopLimitOption)(nil) | |||
|
|||
// TCPOptions represents options for TCP connections. | |||
type TCPOptions interface { | |||
HasWaitUntilBytesAreSent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't have such high-level functionality here. As mentioned below, we should perhaps expose lower level functionality instead.
x/sockopt/sockopt.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about having TCPOptions have options that are not cross-platform. I'm still trying to figure out what a good design for this stuff is, and we'll only know better when we try different things.
In favor of simpler APIs, for now, let's go with a simple standalone function BytesToSend(Rawconn)
that lives in the fake
code, since that's the only strategy that really needs it. It shouldn't be defined in platforms that don't support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that lives in the fake code, since that's the only strategy that really needs it. It shouldn't be defined in platforms that don't support it.
at least on linux, more strategies need it (split, esp. when it performs multiple small splits, disorder)
9a288f7
to
e3d398e
Compare
Fixed some comments. For more fixes, I'd like more input. Waiting for the socket to send all bytes is useful for pretty much all strategies.
We'll use waiting when possible. And when it's not available on the platform we either wait for a defined duration, or don't wait at all. |
64afbfd
to
fe99d36
Compare
Did a bit of debugging of
|
fe99d36
to
3f82afb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the waitstream
approach! It's a nice way to compose. But let's decouple from the TCPOptions. That's intended for low-level portable options.
x/sockopt/is_sending_bytes_linux.go
Outdated
} | ||
|
||
// 1 == TCP_ESTABLISHED, but for some reason not available in the package | ||
if tcpInfo.State != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying we can remove the State
test. It's not doing anything. tcpInfo.Notsent_bytes
will be zero if it's not established.
x/wait_stream/writer.go
Outdated
written, err = w.conn.Write(data) | ||
|
||
// This may not be implemented, so it's best effort really. | ||
waitUntilBytesAreSentErr := w.tcpOptions.WaitUntilBytesAreSent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be before the Write is done.
Also, this may significantly slow down communication. What's the performance impact? We will need to understand that for practical use.
We may want to restrict the wait for only when it's needed (on splits).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used a fetch-speed
tool:
go run *.go -timeout 100500s -transport "waitstream" http://speedtest.belwue.net/100M
Downloaded 100.00 MiB in 9.01s
Downloaded Speed: 11.10 MiB/s
Baseline:
go run *.go -timeout 100500s -transport "" http://speedtest.belwue.net/100M
Downloaded 100.00 MiB in 9.75s
Downloaded Speed: 10.26 MiB/s
This will be helpful for some strategies: