Skip to content

Commit

Permalink
SPDY4 Settings ACK support
Browse files Browse the repository at this point in the history
Update SpdyFramer/SpdyDispatcher to send a SETTINGS ACK after processing a
SETTINGS frame, and receive SETTINGS ACK frames without choking.

This lands server change 61532046 by mlavan.

BUG=345769

Review URL: https://codereview.chromium.org/181613002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@253880 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
jgraettinger@chromium.org committed Feb 27, 2014
1 parent 484f84a commit 0623016
Show file tree
Hide file tree
Showing 13 changed files with 161 additions and 26 deletions.
8 changes: 8 additions & 0 deletions net/quic/quic_headers_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,14 @@ class QuicHeadersStream::SpdyFramerVisitor
CloseConnection("SPDY SETTINGS frame recevied.");
}

virtual void OnSettingsAck() OVERRIDE {
CloseConnection("SPDY SETTINGS frame recevied.");
}

virtual void OnSettingsEnd() OVERRIDE {
CloseConnection("SPDY SETTINGS frame recevied.");
}

virtual void OnPing(SpdyPingId unique_id, bool is_ack) OVERRIDE {
CloseConnection("SPDY PING frame recevied.");
}
Expand Down
2 changes: 2 additions & 0 deletions net/quic/quic_headers_stream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ class MockVisitor : public SpdyFramerVisitorInterface {
SpdyRstStreamStatus status));
MOCK_METHOD1(OnSettings, void(bool clear_persisted));
MOCK_METHOD3(OnSetting, void(SpdySettingsIds id, uint8 flags, uint32 value));
MOCK_METHOD0(OnSettingsAck, void());
MOCK_METHOD0(OnSettingsEnd, void());
MOCK_METHOD2(OnPing, void(SpdyPingId unique_id, bool is_ack));
MOCK_METHOD2(OnGoAway, void(SpdyStreamId last_accepted_stream_id,
SpdyGoAwayStatus status));
Expand Down
2 changes: 2 additions & 0 deletions net/quic/quic_spdy_decompressor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class SpdyFramerVisitor : public SpdyFramerVisitorInterface {
virtual void OnSetting(SpdySettingsIds id,
uint8 flags,
uint32 value) OVERRIDE {}
virtual void OnSettingsAck() OVERRIDE {}
virtual void OnSettingsEnd() OVERRIDE {}
virtual void OnPing(SpdyPingId unique_id, bool is_ack) OVERRIDE {}
virtual void OnGoAway(SpdyStreamId last_accepted_stream_id,
SpdyGoAwayStatus status) OVERRIDE {}
Expand Down
8 changes: 8 additions & 0 deletions net/spdy/buffered_spdy_framer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ void BufferedSpdyFramer::OnSetting(SpdySettingsIds id,
visitor_->OnSetting(id, flags, value);
}

void BufferedSpdyFramer::OnSettingsAck() {
visitor_->OnSettingsAck();
}

void BufferedSpdyFramer::OnSettingsEnd() {
visitor_->OnSettingsEnd();
}

void BufferedSpdyFramer::OnPing(SpdyPingId unique_id, bool is_ack) {
visitor_->OnPing(unique_id, is_ack);
}
Expand Down
8 changes: 8 additions & 0 deletions net/spdy/buffered_spdy_framer.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ class NET_EXPORT_PRIVATE BufferedSpdyFramerVisitorInterface {
// and validated.
virtual void OnSetting(SpdySettingsIds id, uint8 flags, uint32 value) = 0;

// Called when a SETTINGS frame is received with the ACK flag set.
virtual void OnSettingsAck() {}

// Called at the completion of parsing SETTINGS id and value tuples.
virtual void OnSettingsEnd() {};

// Called when a PING frame has been parsed.
virtual void OnPing(SpdyPingId unique_id, bool is_ack) = 0;

Expand Down Expand Up @@ -140,6 +146,8 @@ class NET_EXPORT_PRIVATE BufferedSpdyFramer
virtual void OnSettings(bool clear_persisted) OVERRIDE;
virtual void OnSetting(
SpdySettingsIds id, uint8 flags, uint32 value) OVERRIDE;
virtual void OnSettingsAck() OVERRIDE;
virtual void OnSettingsEnd() OVERRIDE;
virtual void OnPing(SpdyPingId unique_id, bool is_ack) OVERRIDE;
virtual void OnRstStream(SpdyStreamId stream_id,
SpdyRstStreamStatus status) OVERRIDE;
Expand Down
1 change: 1 addition & 0 deletions net/spdy/mock_spdy_framer_visitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class MockSpdyFramerVisitor : public SpdyFramerVisitorInterface {
MOCK_METHOD1(OnSettings, void(bool clear_persisted));
MOCK_METHOD3(OnSetting, void(SpdySettingsIds id, uint8 flags, uint32 value));
MOCK_METHOD2(OnPing, void(SpdyPingId unique_id, bool is_ack));
MOCK_METHOD0(OnSettingsEnd, void());
MOCK_METHOD2(OnGoAway, void(SpdyStreamId last_accepted_stream_id,
SpdyGoAwayStatus status));
MOCK_METHOD2(OnHeaders, void(SpdyStreamId stream_id, bool fin));
Expand Down
2 changes: 1 addition & 1 deletion net/spdy/spdy_frame_builder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ TEST_P(SpdyFrameBuilderTest, RewriteLength) {
SpdyFrameBuilder builder(expected->size() + 1);
if (spdy_version_ < 4) {
builder.WriteControlFrameHeader(framer, SETTINGS, 0);
builder.WriteUInt32(0); // Write the number of settings.
} else {
builder.WriteFramePrefix(framer, SETTINGS, 0, 0);
}
builder.WriteUInt32(0); // Write the number of settings.
EXPECT_TRUE(builder.GetWritableBuffer(1) != NULL);
builder.RewriteLength(framer);
scoped_ptr<SpdyFrame> built(builder.take());
Expand Down
53 changes: 43 additions & 10 deletions net/spdy/spdy_framer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,11 @@ size_t SpdyFramer::GetSettingsMinimumSize() const {
// Size, in bytes, of a SETTINGS frame not including the IDs and values
// from the variable-length value block. Calculated as:
// control frame header + 4 (number of ID/value pairs)
return GetControlFrameHeaderSize() + 4;
if (spdy_version_ < 4) {
return GetControlFrameHeaderSize() + 4;
} else {
return GetControlFrameHeaderSize();
}
}

size_t SpdyFramer::GetPingSize() const {
Expand Down Expand Up @@ -740,18 +744,30 @@ void SpdyFramer::ProcessControlFrameHeader(uint16 control_frame_type_field) {
}
break;
case SETTINGS:
{
// Make sure that we have an integral number of 8-byte key/value pairs,
// plus a 4-byte length field.
// plus a 4-byte length field in SPDY3 and below.
size_t values_prefix_size = (protocol_version() < 4 ? 4 : 0);
if (current_frame_length_ < GetSettingsMinimumSize() ||
(current_frame_length_ - GetControlFrameHeaderSize()) % 8 != 4) {
(current_frame_length_ - GetControlFrameHeaderSize()) % 8 !=
values_prefix_size) {
DLOG(WARNING) << "Invalid length for SETTINGS frame: "
<< current_frame_length_;
set_error(SPDY_INVALID_CONTROL_FRAME);
} else if (current_frame_flags_ &
} else if (protocol_version() < 4 &&
current_frame_flags_ &
~SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS) {
set_error(SPDY_INVALID_CONTROL_FRAME_FLAGS);
} else if (protocol_version() >= 4 &&
current_frame_flags_ & ~SETTINGS_FLAG_ACK) {
set_error(SPDY_INVALID_CONTROL_FRAME_FLAGS);
} else if (protocol_version() >= 4 &&
current_frame_flags_ & SETTINGS_FLAG_ACK &&
current_frame_length_ > GetSettingsMinimumSize()) {
set_error(SPDY_INVALID_CONTROL_FRAME);
}
break;
}
case PING:
if (current_frame_length_ != GetPingSize()) {
set_error(SPDY_INVALID_CONTROL_FRAME);
Expand Down Expand Up @@ -1191,9 +1207,14 @@ size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
CHANGE_STATE(SPDY_CONTROL_FRAME_HEADER_BLOCK);
break;
case SETTINGS:
visitor_->OnSettings(current_frame_flags_ &
SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS);
CHANGE_STATE(SPDY_SETTINGS_FRAME_PAYLOAD);
if (spdy_version_ >= 4 && current_frame_flags_ & SETTINGS_FLAG_ACK) {
visitor_->OnSettingsAck();
CHANGE_STATE(SPDY_AUTO_RESET);
} else {
visitor_->OnSettings(current_frame_flags_ &
SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS);
CHANGE_STATE(SPDY_SETTINGS_FRAME_PAYLOAD);
}
break;
case SYN_REPLY:
case HEADERS:
Expand Down Expand Up @@ -1386,6 +1407,7 @@ size_t SpdyFramer::ProcessSettingsFramePayload(const char* data,
// Check if we're done handling this SETTINGS frame.
remaining_data_length_ -= processed_bytes;
if (remaining_data_length_ == 0) {
visitor_->OnSettingsEnd();
CHANGE_STATE(SPDY_AUTO_RESET);
}

Expand Down Expand Up @@ -1418,7 +1440,7 @@ bool SpdyFramer::ProcessSetting(const char* data) {
}
SpdySettingsIds id = static_cast<SpdySettingsIds>(id_and_flags.id());

// Detect duplciates.
// Detect duplicates.
if (static_cast<uint32>(id) <= settings_scratch_.last_setting_id) {
DLOG(WARNING) << "Duplicate entry or invalid ordering for id " << id
<< " in " << display_protocol_ << " SETTINGS frame "
Expand Down Expand Up @@ -1909,9 +1931,12 @@ SpdySerializedFrame* SpdyFramer::SerializeRstStream(
SpdySerializedFrame* SpdyFramer::SerializeSettings(
const SpdySettingsIR& settings) const {
uint8 flags = 0;
if (settings.clear_settings()) {
if (spdy_version_ < 4 && settings.clear_settings()) {
flags |= SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS;
}
if (spdy_version_ >= 4 && settings.is_ack()) {
flags |= SETTINGS_FLAG_ACK;
}
const SpdySettingsIR::ValueMap* values = &(settings.values());

// Size, in bytes, of this SETTINGS frame.
Expand All @@ -1923,7 +1948,15 @@ SpdySerializedFrame* SpdyFramer::SerializeSettings(
} else {
builder.WriteFramePrefix(*this, SETTINGS, flags, 0);
}
builder.WriteUInt32(values->size());

// If this is an ACK, payload should be empty.
if (spdy_version_ >= 4 && settings.is_ack()) {
return builder.take();
}

if (spdy_version_ < 4) {
builder.WriteUInt32(values->size());
}
DCHECK_EQ(GetSettingsMinimumSize(), builder.length());
for (SpdySettingsIR::ValueMap::const_iterator it = values->begin();
it != values->end();
Expand Down
6 changes: 6 additions & 0 deletions net/spdy/spdy_framer.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,12 @@ class NET_EXPORT_PRIVATE SpdyFramerVisitorInterface {
// validated.
virtual void OnSetting(SpdySettingsIds id, uint8 flags, uint32 value) = 0;

// Called when a SETTINGS frame is received with the ACK flag set.
virtual void OnSettingsAck() {}

// Called before and after parsing SETTINGS id and value tuples.
virtual void OnSettingsEnd() = 0;

// Called when a PING frame has been parsed.
virtual void OnPing(SpdyPingId unique_id, bool is_ack) = 0;

Expand Down
Loading

0 comments on commit 0623016

Please sign in to comment.