Net: Defer reading until listeners could be added by jameshartig · Pull Request #1496 · nodejs/node

jameshartig · 2015-04-22T01:05:04Z

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

jameshartig · 2015-04-22T01:09:56Z

This also fixes nodejs/node-v0.x-archive#9355

chrisdickinson · 2015-04-22T04:03:04Z

lib/_tls_wrap.js

I know this has just been moved, but we really need to standardize an API for querying the size of a stream's buffer.

jameshartig · 2015-04-22T19:54:21Z

@chrisdickinson Updated to send along this as an arg to nextTick and added a check for if (!self._handle). I also removed ended from the tests that @brendanashworth brought up. Since we're not using unref we don't need to check that it ended since the process won't die until it has.

Thanks guys for taking a look at this so quickly.

chrisdickinson · 2015-04-22T21:37:17Z

lib/_tls_wrap.js

could you name this anonymous function and pull it out of the constructor?

Yup! Updated and moved into a named function above the constructor called initread

jameshartig · 2015-04-27T19:35:18Z

@chrisdickinson I moved the anonymous function to above the constructor. Everything else look good? Thanks again!

chrisdickinson · 2015-04-27T19:49:57Z

LGTM! If @indutny does not object, I'll merge this tomorrow.

indutny · 2015-04-27T19:56:39Z

lib/_tls_wrap.js

Camel case, please.

Gotcha. I was trying to follow the function above onnewsession which was all lowercase so I thought that was the standard :/

jameshartig · 2015-04-27T20:45:47Z

@indutny I renamed to initRead. If you want me to keep the ended var and checks in the tests, I can add those back, just let me know. Thanks

jameshartig · 2015-05-03T02:53:08Z

@indutny would you mind taking another look after the changes I made? Thanks!

jameshartig · 2015-05-27T05:27:52Z

Any updates @indutny @chrisdickinson ?

indutny · 2015-05-27T10:47:32Z

LGTM, @chrisdickinson please land!

brendanashworth · 2015-06-08T19:20:48Z

test/parallel/test-tls-delayed-attach-error.js

A 'use strict'; now needs to be added here with the new linter.

jameshartig · 2015-06-08T22:25:35Z

@brendanashworth updated with 'use strict'; and linter seems to like it and tests still pass.

@chrisdickinson good to go?

brendanashworth · 2015-06-16T00:59:01Z

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/10/

brendanashworth · 2015-06-16T01:01:17Z

hmm... the linter job failed. @fastest963 could you rebase your remote branch onto master and push to GH (preferably with a make lint beforehand)? It might need an update.

jameshartig · 2015-06-16T01:22:16Z

@brendanashworth It looks like it failed because it couldn't find ./iojs:

[linter] $ /bin/sh -xe /tmp/hudson2733538223575635564.sh
+ NODE=/usr/local/bin/iojs make lint
gmake[1]: Entering directory '/usr/home/iojs/build/workspace/iojs+linter/nodes/linter'
./iojs tools/eslint/bin/eslint.js src lib test --reset --quiet
gmake[1]: ./iojs: Command not found
Makefile:378: recipe for target 'jslint' failed
gmake[1]: *** [jslint] Error 127
gmake[1]: Leaving directory '/usr/home/iojs/build/workspace/iojs+linter/nodes/linter'
*** Error code 2

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

brendanashworth · 2015-06-16T01:50:18Z

I don't work on build but I think you need the recent updates to the
Makefile to lint on the CI. <surprise!> Could you rebase, then I'll try to
run it again.

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:

@brendanashworth https://github.com/brendanashworth It looks like it
failed because of:

[linter] $ /bin/sh -xe /tmp/hudson2733538223575635564.sh

NODE=/usr/local/bin/iojs make lint
gmake[1]: Entering directory '/usr/home/iojs/build/workspace/iojs+linter/nodes/linter'
./iojs tools/eslint/bin/eslint.js src lib test --reset --quiet
gmake[1]: ./iojs: Command not found
Makefile:378: recipe for target 'jslint' failed
gmake[1]: *** [jslint] Error 127
gmake[1]: Leaving directory '/usr/home/iojs/build/workspace/iojs+linter/nodes/linter'
*** Error code 2

—
Reply to this email directly or view it on GitHub
#1496 (comment).

jameshartig · 2015-06-16T01:51:17Z

No problem! I just rebased and pushed, want to try again? Thanks!

brendanashworth · 2015-06-16T03:50:38Z

perfecto: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/14/

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>

brendanashworth · 2015-06-17T21:50:07Z

Thanks for the pull request! Landed in 061342a.

mscdex added the tls Issues and PRs related to the tls subsystem. label Apr 22, 2015

chrisdickinson reviewed Apr 22, 2015
View reviewed changes

jameshartig changed the title ~~Net: Defer reading until callbacks could be added~~ Net: Defer reading until listeners could be added Apr 23, 2015

indutny reviewed Apr 27, 2015
View reviewed changes

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

brendanashworth reviewed Jun 8, 2015
View reviewed changes

test/parallel/test-tls-delayed-attach-error.js

Copy link

Contributor

brendanashworth Jun 8, 2015

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.

Net: Defer reading until listeners could be added

0042c14

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

brendanashworth closed this Jun 17, 2015

brendanashworth mentioned this pull request Jun 17, 2015

Already buffered data when creating a TLSSocket doesn't trigger/throw 'error' #1114

Closed

rvagg mentioned this pull request Jun 18, 2015

Release proposal: v2.3.1 #1996

Closed

snyk-bot mentioned this pull request Jul 14, 2021

[Snyk] Upgrade protobufjs from 6.8.8 to 6.11.2 Zintexiss/node-22#27

Open

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Upgrade protobufjs from 6.8.8 to 6.11.2 Iceymann18777/node#9

Open

MaxMood96 mentioned this pull request May 23, 2022

[Snyk] Security upgrade protobufjs from 6.8.8 to 6.11.3 MaxMood96/node#22

Open

ryan-ally mentioned this pull request May 23, 2022

[Snyk] Security upgrade protobufjs from 6.8.8 to 6.11.3 ryan-ally/node#62

Open

snyk-bot mentioned this pull request Aug 19, 2022

[Snyk] Security upgrade protobufjs from 6.8.8 to 6.11.3 MarcelRaschke/node#1

Open

snyk-bot mentioned this pull request Apr 25, 2023

[Snyk] Security upgrade protobufjs from 6.8.8 to 6.11.3 aliscco/alisco-node#181

Open

leonardoadame mentioned this pull request Jul 8, 2023

[Snyk] Security upgrade protobufjs from 6.8.8 to 6.11.3 leonardoadame/node#106

Open

Uh oh!

Conversation

jameshartig commented Apr 22, 2015

Uh oh!

jameshartig commented Apr 22, 2015

Uh oh!

chrisdickinson Apr 22, 2015

Choose a reason for hiding this comment

Uh oh!

jameshartig commented Apr 22, 2015

Uh oh!

chrisdickinson Apr 22, 2015

Choose a reason for hiding this comment

Uh oh!

jameshartig Apr 23, 2015

Choose a reason for hiding this comment

Uh oh!

jameshartig commented Apr 27, 2015

Uh oh!

chrisdickinson commented Apr 27, 2015

Uh oh!

indutny Apr 27, 2015

Choose a reason for hiding this comment

Uh oh!

jameshartig Apr 27, 2015

Choose a reason for hiding this comment

Uh oh!

jameshartig commented Apr 27, 2015

Uh oh!

jameshartig commented May 3, 2015

Uh oh!

jameshartig commented May 27, 2015

Uh oh!

indutny commented May 27, 2015

Uh oh!

brendanashworth Jun 8, 2015

Choose a reason for hiding this comment

Uh oh!

jameshartig commented Jun 8, 2015

Uh oh!

brendanashworth commented Jun 16, 2015

Uh oh!

brendanashworth commented Jun 16, 2015

Uh oh!

jameshartig commented Jun 16, 2015

Uh oh!

brendanashworth commented Jun 16, 2015

Uh oh!

jameshartig commented Jun 16, 2015

Uh oh!

brendanashworth commented Jun 16, 2015

Uh oh!

brendanashworth commented Jun 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants