Conversation
Fedor, is this still relevant? If so, can you rebase it? |
Rebased. |
Btw, are we going to try it out? /cc @isaacs @bnoordhuis |
} | ||
|
||
if (hello.sessionId.length > 0 && | ||
this.server.listeners('resumeTlsSession').length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Premature optimization, probably even a deoptimization when .listeners()
returns a copy (which is likely going to happen).
EDIT: Hah, sorry - missed the else. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is only a deoptimization. It should be something like this:
if (hello.sessionId.length <= 0 ||
!this.server.emit('resumeTlsSession', hello.sessionId, callback)) {
callback(null, null);
}
emit
returns true if any listeners were called, or false if there are no listeners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Force pushed with @isaacs variant
Fixed everything mentioned. |
Rebased patch on master. |
|
||
Connection* p = static_cast<Connection*>(SSL_get_app_data(s)); | ||
|
||
*copy = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, 0
|
||
if (ss->is_server_ && !ss->hello_parser_.ended()) { | ||
bytes_written = ss->hello_parser_.Write( | ||
reinterpret_cast<uint8_t*>(buffer_data + off), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: needs to align. Maybe use a temp var.
Generally LGTM. Fix the latest round of comments and add a few tests and it's ready to go in. |
Your patch seems to break
|
Fixed nits, tests are passing. |
} | ||
|
||
// Check if we overflowed (do not reply with any private data) | ||
if (session_id == NULL || session_id + session_size > data_ + offset_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe session_size
should always be >=16
and <= 32
. I think its safer to encode these limitations here, and fallback to openssl -- I want to be paranoid about parsing this kind of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fFixed.
One final nit, otherwise LGTM. |
Thank you very much for reviewing this for a lot of times! Landed 8e0c830 |
WIP
Example: