Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(404)

Unified Diff: net/socket/tcp_socket_libevent.cc

Issue 583883002: Adds TCP FastOpen blackhole recovery code to avoid getting persistently blackholed. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Nit fixed. Created 6 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« net/socket/tcp_socket_libevent.h ('K') | « net/socket/tcp_socket_libevent.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: net/socket/tcp_socket_libevent.cc
diff --git a/net/socket/tcp_socket_libevent.cc b/net/socket/tcp_socket_libevent.cc
index f39611b603b63df6774e035017a7bbb60aef0750..df3ba9aced8aa25eed5f11179f8e695a3ee577c2 100644
--- a/net/socket/tcp_socket_libevent.cc
+++ b/net/socket/tcp_socket_libevent.cc
@@ -39,6 +39,9 @@ bool g_tcp_fastopen_supported = false;
// True if TCP FastOpen is user-enabled for all connections.
// TODO(jri): Change global variable to param in HttpNetworkSession::Params.
bool g_tcp_fastopen_user_enabled = false;
+// True if TCP FastOpen connect-with-write has failed at least once.
+bool g_tcp_fastopen_has_failed = false;
+
// SetTCPNoDelay turns on/off buffering in the kernel. By default, TCP sockets
// will wait up to 200ms for more data to complete a packet before transmitting.
@@ -129,8 +132,9 @@ void CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled) {
TCPSocketLibevent::TCPSocketLibevent(NetLog* net_log,
const NetLog::Source& source)
: use_tcp_fastopen_(false),
+ tcp_fastopen_write_attempted_(false),
tcp_fastopen_connected_(false),
- fast_open_status_(FAST_OPEN_STATUS_UNKNOWN),
+ tcp_fastopen_status_(FAST_OPEN_STATUS_UNKNOWN),
logging_multiple_connect_attempts_(false),
net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_SOCKET)) {
net_log_.BeginEvent(NetLog::TYPE_SOCKET_ALIVE,
@@ -139,9 +143,9 @@ TCPSocketLibevent::TCPSocketLibevent(NetLog* net_log,
TCPSocketLibevent::~TCPSocketLibevent() {
net_log_.EndEvent(NetLog::TYPE_SOCKET_ALIVE);
- if (tcp_fastopen_connected_) {
+ if (tcp_fastopen_write_attempted_) {
UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection",
- fast_open_status_, FAST_OPEN_MAX_VALUE);
+ tcp_fastopen_status_, FAST_OPEN_MAX_VALUE);
mmenke 2014/09/23 15:21:00 You need to update histograms.xml in this CL as we
mmenke 2014/09/23 15:21:00 Since you're significantly changing this histogram
Jana 2014/09/24 00:51:26 I thought about that ... but this histogram doesn'
Jana 2014/09/24 00:51:26 I did add histograms.xml to this CL, but to add my
}
}
@@ -222,7 +226,7 @@ int TCPSocketLibevent::Connect(const IPEndPoint& address,
if (use_tcp_fastopen_) {
// With TCP FastOpen, we pretend that the socket is connected.
- DCHECK(!tcp_fastopen_connected_);
+ DCHECK(!tcp_fastopen_write_attempted_);
socket_->SetPeerAddress(storage);
return OK;
}
@@ -239,7 +243,7 @@ bool TCPSocketLibevent::IsConnected() const {
if (!socket_)
return false;
- if (use_tcp_fastopen_ && !tcp_fastopen_connected_ &&
+ if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_ &&
socket_->HasPeerAddress()) {
// With TCP FastOpen, we pretend that the socket is connected.
// This allows GetPeerAddress() to return peer_address_.
@@ -269,7 +273,7 @@ int TCPSocketLibevent::Read(IOBuffer* buf,
// ownership of buf to socket.
base::Unretained(this), make_scoped_refptr(buf), callback));
if (rv >= 0)
- RecordFastOpenStatus();
+ RecordTCPFastOpenStatus();
if (rv != ERR_IO_PENDING)
rv = HandleReadCompleted(buf, rv);
return rv;
@@ -288,7 +292,8 @@ int TCPSocketLibevent::Write(IOBuffer* buf,
// ownership of buf to socket.
base::Unretained(this), make_scoped_refptr(buf), callback);
int rv;
- if (use_tcp_fastopen_ && !tcp_fastopen_connected_) {
+
+ if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_) {
rv = TcpFastOpenWrite(buf, buf_len, write_callback);
} else {
rv = socket_->Write(buf, buf_len, write_callback);
@@ -415,7 +420,7 @@ bool TCPSocketLibevent::SetNoDelay(bool no_delay) {
void TCPSocketLibevent::Close() {
socket_.reset();
tcp_fastopen_connected_ = false;
- fast_open_status_ = FAST_OPEN_STATUS_UNKNOWN;
+ tcp_fastopen_status_ = FAST_OPEN_STATUS_UNKNOWN;
}
bool TCPSocketLibevent::UsingTCPFastOpen() const {
@@ -423,7 +428,11 @@ bool TCPSocketLibevent::UsingTCPFastOpen() const {
}
void TCPSocketLibevent::EnableTCPFastOpenIfSupported() {
- if (IsTCPFastOpenSupported())
+ // Do not enable TCP FastOpen if it had previously failed.
+ // This check conservatively avoids middleboxes that may blackhole
+ // TCP FastOpen SYN+Data packets; on such a failure, subsequent sockets
+ // should not use TCP FastOpen.
+ if (IsTCPFastOpenSupported() && !g_tcp_fastopen_has_failed)
use_tcp_fastopen_ = true;
}
@@ -556,7 +565,7 @@ void TCPSocketLibevent::ReadCompleted(const scoped_refptr<IOBuffer>& buf,
// Records TCP FastOpen status regardless of error in asynchronous case.
// TODO(rdsmith,jri): Change histogram name to indicate it could be called on
// error.
- RecordFastOpenStatus();
+ RecordTCPFastOpenStatus();
callback.Run(HandleReadCompleted(buf.get(), rv));
}
@@ -576,18 +585,25 @@ int TCPSocketLibevent::HandleReadCompleted(IOBuffer* buf, int rv) {
void TCPSocketLibevent::WriteCompleted(const scoped_refptr<IOBuffer>& buf,
const CompletionCallback& callback,
- int rv) const {
+ int rv) {
DCHECK_NE(ERR_IO_PENDING, rv);
callback.Run(HandleWriteCompleted(buf.get(), rv));
}
-int TCPSocketLibevent::HandleWriteCompleted(IOBuffer* buf, int rv) const {
+int TCPSocketLibevent::HandleWriteCompleted(IOBuffer* buf, int rv) {
if (rv < 0) {
net_log_.AddEvent(NetLog::TYPE_SOCKET_WRITE_ERROR,
CreateNetLogSocketErrorCallback(rv, errno));
+ if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
+ // TCP FastOpen connect-with-write was attempted, but failed.
+ // Turn off TCP FastOpen for any subsequent connections.
+ g_tcp_fastopen_has_failed = true;
+ RecordTCPFastOpenStatus();
+ }
return rv;
}
+ tcp_fastopen_connected_ = true;
base::StatsCounter write_bytes("tcp.write_bytes");
write_bytes.Add(rv);
net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_SENT, rv,
@@ -618,10 +634,10 @@ int TCPSocketLibevent::TcpFastOpenWrite(
flags,
storage.addr,
storage.addr_len));
- tcp_fastopen_connected_ = true;
+ tcp_fastopen_write_attempted_ = true;
if (rv >= 0) {
- fast_open_status_ = FAST_OPEN_FAST_CONNECT_RETURN;
+ tcp_fastopen_status_ = FAST_OPEN_FAST_CONNECT_RETURN;
return rv;
}
@@ -639,19 +655,28 @@ int TCPSocketLibevent::TcpFastOpenWrite(
}
if (rv != ERR_IO_PENDING) {
- fast_open_status_ = FAST_OPEN_ERROR;
+ tcp_fastopen_status_ = FAST_OPEN_ERROR;
return rv;
}
- fast_open_status_ = FAST_OPEN_SLOW_CONNECT_RETURN;
+ tcp_fastopen_status_ = FAST_OPEN_SLOW_CONNECT_RETURN;
return socket_->WaitForWrite(buf, buf_len, callback);
}
-void TCPSocketLibevent::RecordFastOpenStatus() {
+void TCPSocketLibevent::RecordTCPFastOpenStatus() {
mmenke 2014/09/23 15:21:00 nit: Think "Record" is a bit of a misnomer, since
Jana 2014/09/24 00:51:26 Sure, but we are recording the status here locally
if (use_tcp_fastopen_ &&
- (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN ||
- fast_open_status_ == FAST_OPEN_SLOW_CONNECT_RETURN)) {
- DCHECK_NE(FAST_OPEN_STATUS_UNKNOWN, fast_open_status_);
+ (tcp_fastopen_status_ == FAST_OPEN_FAST_CONNECT_RETURN ||
mmenke 2014/09/23 15:21:00 nit: Should be consistent here: Either always us
Jana 2014/09/24 00:51:26 Yup, good call. I left this one as it was (this hi
+ tcp_fastopen_status_ == FAST_OPEN_SLOW_CONNECT_RETURN)) {
mmenke 2014/09/23 15:21:00 While you're here, would you mind switching to an
Jana 2014/09/24 00:51:27 Done.
+ DCHECK_NE(FAST_OPEN_STATUS_UNKNOWN, tcp_fastopen_status_);
+
+ if (g_tcp_fastopen_has_failed && tcp_fastopen_write_attempted_) {
+ // Writing TCP FastOpen SYN+DATA or SYN-option failed.
+ tcp_fastopen_status_ =
+ (tcp_fastopen_status_ == FAST_OPEN_FAST_CONNECT_RETURN ?
+ FAST_OPEN_FAST_CONNECT_FAILED : FAST_OPEN_SLOW_CONNECT_FAILED);
mmenke 2014/09/23 15:21:00 If g_tcp_fastopen_has_failed is true, you're assum
Jana 2014/09/24 00:51:26 I don't think so -- but I've streamlined the logic
+ return;
+ }
+
bool getsockopt_success(false);
bool server_acked_data(false);
#if defined(TCP_INFO)
@@ -666,17 +691,17 @@ void TCPSocketLibevent::RecordFastOpenStatus() {
(info.tcpi_options & TCPI_OPT_SYN_DATA);
#endif
if (getsockopt_success) {
- if (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN) {
- fast_open_status_ = (server_acked_data ? FAST_OPEN_SYN_DATA_ACK :
+ if (tcp_fastopen_status_ == FAST_OPEN_FAST_CONNECT_RETURN) {
+ tcp_fastopen_status_ = (server_acked_data ? FAST_OPEN_SYN_DATA_ACK :
FAST_OPEN_SYN_DATA_NACK);
} else {
- fast_open_status_ = (server_acked_data ? FAST_OPEN_NO_SYN_DATA_ACK :
+ tcp_fastopen_status_ = (server_acked_data ? FAST_OPEN_NO_SYN_DATA_ACK :
FAST_OPEN_NO_SYN_DATA_NACK);
}
} else {
- fast_open_status_ = (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN ?
- FAST_OPEN_SYN_DATA_FAILED :
- FAST_OPEN_NO_SYN_DATA_FAILED);
+ tcp_fastopen_status_ =
+ (tcp_fastopen_status_ == FAST_OPEN_FAST_CONNECT_RETURN ?
+ FAST_OPEN_SYN_DATA_FAILED : FAST_OPEN_NO_SYN_DATA_FAILED);
}
}
}
« net/socket/tcp_socket_libevent.h ('K') | « net/socket/tcp_socket_libevent.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698