Concatenate header and first upstream payload #73
Conversation
When possible, this change concatenates the Shadowsocks header and the first upstream payload into a single TCP segment. This change imposes additional memory (extra buffer) and time (lock acquisition) costs, but only during the first read.
shadowsocks/client.go
Outdated
| // We therefore use a short delay, longer than any reasonable IPC but similar to | ||
| // typical network latency. (In an Android emulator, the 90th percentile delay | ||
| // was ~1 ms.) If no client payload is received by this time, we connect without it. | ||
| const helloWait = 20 * time.Millisecond |
There was a problem hiding this comment.
How did you decide no 20 ms?
This will only delay connections where the client doesn't immediately send data, but I have no idea what those are, so maybe reduce the wait time?
There was a problem hiding this comment.
OK, reduced to 10ms
shadowsocks/client_test.go
Outdated
| t.Fatalf("ShadowsocksClient.DialTCP failed: %v", err) | ||
| } | ||
|
|
||
| // Wait for more than 20 milliseconds to ensure that the target |
There was a problem hiding this comment.
Should we check that no data was sent in the first 20ms?
There was a problem hiding this comment.
OK, I added a test for this.
| LazyWrite(p []byte) (int, error) | ||
| // Flush sends the pending data, if any. This method is | ||
| // thread-safe, but must not be the first method called. | ||
| Flush() error |
There was a problem hiding this comment.
I don't get the "MUST". Why can't it be called first? Wouldn't it just return and do nothing? I don't see what would break.
There was a problem hiding this comment.
Flush() relies on init() having run, but it can't run init() because init() isn't thread-safe.
There was a problem hiding this comment.
When there was no write, you only depend on needFlush and the mutex, both of which are initialized before init(). A sequence Flush, LazyWrite, Write seems safe.
There was a problem hiding this comment.
You're right. Done.
shadowsocks/stream.go
Outdated
| // modified on the flush thread. | ||
| // Any size is acceptable here, but this is the largest value | ||
| // for which a single call to enqueue() is sufficient. | ||
| readBuf := make([]byte, len(payloadBuf)-pending) |
There was a problem hiding this comment.
You don't need this allocation.
You can use sw.buf[saltSize + 2 + sw.aead,Overhead() + sw.pending + sw.aead.Overhead():]
The enqueue call later will shift things to the right position.
There was a problem hiding this comment.
Beautiful. Done.
shadowsocks/stream.go
Outdated
| sw.mu.Lock() | ||
|
|
||
| sw.enqueue(readBuf[:plaintextSize]) | ||
| readBuf = nil // Release memory before blocking I/O. |
There was a problem hiding this comment.
This can be deleted if you remove the make call
There was a problem hiding this comment.
Done
| LazyWrite(p []byte) (int, error) | ||
| // Flush sends the pending data, if any. This method is | ||
| // thread-safe, but must not be the first method called. | ||
| Flush() error |
There was a problem hiding this comment.
When there was no write, you only depend on needFlush and the mutex, both of which are initialized before init(). A sequence Flush, LazyWrite, Write seems safe.
| if err != nil { | ||
| t.Errorf("Write failed: %v", err) | ||
| } | ||
| if buf.Len() == len1 { |
There was a problem hiding this comment.
Also add a check that buf.Len() == 8 to show the first write got flushed
There was a problem hiding this comment.
I also suggest you make the second write be 3 or 5 bytes, so it's not confused with the first write.
There was a problem hiding this comment.
OK, done
|
Thanks for this change. I'm hopeful it will help disguise server access. Hopefully we'll see a decrease in probes. |
When possible, this change concatenates the Shadowsocks header and the
first upstream payload into a single TCP segment.
This change imposes additional memory (extra buffer) and time (lock
acquisition) costs, but only during the first read.
The text was updated successfully, but these errors were encountered: