Cleanup: Signal READ_READY event only if the buffer is readable#5826
Cleanup: Signal READ_READY event only if the buffer is readable#5826masaori335 merged 1 commit intoapache:masterfrom
Conversation
READ_READY event is always signaled itself when Http2ClientSession is started. But this could waste some CPU cycle if there no data to read.
masaori335
force-pushed
the
h2-ssn-cleanup-0
branch
from
5a1b88e to
3cc0e28
Compare
| 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)) { |
There was a problem hiding this comment.
This implicitly says we have something to do if we can read 1 byte. What is it?
There was a problem hiding this comment.
May be Connection Preface ?
There was a problem hiding this comment.
Do we want the first byte of Connection Preface here? The length should be 10 bytes or so.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds like we could remove this since nobody cares cleartext H2. That would be more simpler.
There was a problem hiding this comment.
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.
shinrich
left a comment
shinrich
left a comment
There was a problem hiding this comment.
Looks good to me. Should protect from spurious read ready events.
|
[approve ci autest] |
|
Fixed the client_context_dump test in PR #5828. Don't know why the order would have changed, but the implementation is not assuming any order so seems reasonable to fix the test to look for content and rely on dump order. |
|
[approve ci autest] |
|
Merged #5828, I hope autest job success. |
|
Looks #5828 fixed the autest job issue. ( or I'm just lucky ) |
4 participants
READ_READY event is always signaled itself when Http2ClientSession is started.
But this could waste some CPU cycle if there no data to read.