Skip to content

Cleanup: Signal READ_READY event only if the buffer is readable #5826

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.

Sign up for GitHub

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

Merged
masaori335 merged 1 commit into from
Aug 15, 2019
Merged

Cleanup: Signal READ_READY event only if the buffer is readable #5826

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions proxy/http2/Http2ClientSession.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,15 +165,13 @@ Http2ClientSession::free()
void
Http2ClientSession::start()
{
VIO *read_vio;

SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());

SET_HANDLER(&Http2ClientSession::main_event_handler);
HTTP2_SET_SESSION_HANDLER(&Http2ClientSession::state_read_connection_preface);

read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer);
write_vio = this->do_io_write(this, INT64_MAX, this->sm_writer);
VIO *read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer);
write_vio = this->do_io_write(this, INT64_MAX, this->sm_writer);

// 3.5 HTTP/2 Connection Preface. Upon establishment of a TCP connection and
// determination that HTTP/2 will be used by both peers, each endpoint MUST
Expand All @@ -183,7 +181,10 @@ Http2ClientSession::start()

this->connection_state.init();
send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_INIT, this);
this->handleEvent(VC_EVENT_READ_READY, read_vio);

if (this->sm_reader->is_read_avail_more_than(0)) {
Copy link
Member

@maskit maskit Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implicitly says we have something to do if we can read 1 byte. What is it?

Copy link
Contributor Author

@masaori335 masaori335 Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be Connection Preface ?

Copy link
Member

@maskit maskit Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the first byte of Connection Preface here? The length should be 10 bytes or so.

Copy link
Contributor Author

@masaori335 masaori335 Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not put these assumption here to make the logic simple.

Think about the error cases, if we start reading with 1 byte, the handler can reject the connection with malformed strings smaller than 10 bytes. ( actually, current state_read_connection_preface() doesn't do this )

Also, in the HTTP/2 over TLS case this is 0.

Copy link
Member

@maskit maskit Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we could remove this since nobody cares cleartext H2. That would be more simpler.

Copy link
Contributor Author

@masaori335 masaori335 Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my first idea, but I'm not confident because there is a path of the buffer is given to the the Http2SessionAccept from the outside.

this->handleEvent(VC_EVENT_READ_READY, read_vio);
}
}

void
Expand Down