-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
Net: Defer reading until listeners could be added #1496
Conversation
This also fixes nodejs/node-v0.x-archive#9355 |
var self = this; | ||
process.nextTick(function() { | ||
// Socket already has some buffered data - emulate receiving it | ||
if (socket && socket._readableState && socket._readableState.length) { |
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 know this has just been moved, but we really need to standardize an API for querying the size of a stream's buffer.
@chrisdickinson Updated to send along Thanks guys for taking a look at this so quickly. |
} | ||
|
||
self.read(0); | ||
}, this); |
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.
could you name this anonymous function and pull it out of the constructor?
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.
Yup! Updated and moved into a named function above the constructor called initread
@chrisdickinson I moved the anonymous function to above the constructor. Everything else look good? Thanks again! |
LGTM! If @indutny does not object, I'll merge this tomorrow. |
@@ -199,6 +199,20 @@ function onocspresponse(resp) { | |||
this.emit('OCSPResponse', resp); | |||
} | |||
|
|||
function initread(tls, wrapped) { |
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.
Camel case, please.
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.
Gotcha. I was trying to follow the function above onnewsession
which was all lowercase so I thought that was the standard :/
@indutny I renamed to |
@indutny would you mind taking another look after the changes I made? Thanks! |
Any updates @indutny @chrisdickinson ? |
LGTM, @chrisdickinson please land! |
@@ -0,0 +1,45 @@ | |||
var common = require('../common'); |
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.
A 'use strict';
now needs to be added here with the new linter.
@brendanashworth updated with @chrisdickinson good to go? |
hmm... the linter job failed. @fastest963 could you rebase your remote branch onto master and push to GH (preferably with a |
@brendanashworth It looks like it failed because it couldn't find
|
Defer reading until user-land has a chance to add listeners. This allows the TLS wrapper to listen for _tlsError and trigger a clientError event if the socket already has data that could trigger. Fixes #1114
I don't work on build but I think you need the recent updates to the Sorry about this, its a new CI setup. I'm still getting used to it. On Monday, June 15, 2015, James Hartig notifications@github.com wrote:
|
No problem! I just rebased and pushed, want to try again? Thanks! |
Defer reading until user-land has a chance to add listeners. This allows the TLS wrapper to listen for _tlsError and trigger a clientError event if the socket already has data that could trigger. Fixes: #1114 PR-URL: #1496 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Thanks for the pull request! Landed in 061342a. |
Defer reading until user-land has a chance to add listeners. This
allows the TLS wrapper to listen for _tlsError and trigger a
clientError event if the socket already has data that could trigger.
Fixes #1114