Skip to content
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

Jump to bottom

crypto: make LazyTransform compabile with Streams1 #12380

Closed
wants to merge 1 commit into from
Closed

crypto: make LazyTransform compabile with Streams1 #12380

wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto, streams

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem. labels Apr 13, 2017
@sam-github
Copy link
Contributor

s/compabile/compatible/

cjihrig
cjihrig requested changes Apr 13, 2017
View reviewed changes
test/parallel/test-crypto-lazy-transform-writable.js Outdated
const hasher1 = crypto.createHash('sha256');
const hasher2 = crypto.createHash('sha256');

// calcualting the expected result
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to // Calculate the expected result.

test/parallel/test-crypto-lazy-transform-writable.js Outdated

stream.pipe(hasher2).on('finish', common.mustCall(function() {
const hash = hasher2.read().toString('hex');
assert.strictEqual(expected, hash, 'the hash matches');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the third parameter should say the hash doesn't match.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just my two bits, but I think most description strings are a waste of time and space. The reported on failure line number leads directly to the assert code, which is quite readable. As-is, the string just repeats the arg name and the word "match", which is a synonym for "equal", so its just a comment that restates exactly what the code says, a classic anti-pattern.

@mcollina
crypto: make LazyTransform compabile with Streams1
06844ff 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs#12269
@mcollina
Copy link
Member Author

@cjihrig @sam-github updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/7397/

@mcollina
Copy link
Member Author

Landed in 1ae172b

@mcollina mcollina closed this Apr 14, 2017
@mcollina mcollina deleted the lazy-transform branch April 14, 2017 14:17
mcollina added a commit that referenced this pull request Apr 14, 2017
@mcollina
crypto: make LazyTransform compabile with Streams1
1ae172b 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
ebc69be 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
18dbc1c 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
8ac8e50 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

@mcollina I've backported to v6.x-staging please lmk if you think this was a mistake and we can rebase it out.

MylesBorins pushed a commit that referenced this pull request May 15, 2017
@mcollina @MylesBorins
crypto: make LazyTransform compabile with Streams1
b1dc123 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins added land-on-v6.x notable-change PRs with changes that should be highlighted in changelogs. labels May 15, 2017
@mcollina
Copy link
Member Author

@MylesBorins you did well, it should be backported.

MylesBorins pushed a commit that referenced this pull request May 18, 2017
@mcollina @MylesBorins
crypto: make LazyTransform compabile with Streams1
0a79b2d 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
MylesBorins added a commit that referenced this pull request Jun 6, 2017
@MylesBorins
2017-06-06, Version 6.11.0 'Boron' (LTS)
40859e2 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    #10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    #10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    #8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    #8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    #9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    #11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    #12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    #11094
  - upgrade libuv to 1.10.2 (cjihrig)
    #10717
  - upgrade libuv to 1.10.1 (cjihrig)
    #9647
  - upgrade libuv to 1.10.0 (cjihrig)
    #9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    #9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    #10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    #10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    #11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    #10294

PR-URL: #13059
MylesBorins added a commit that referenced this pull request Jun 6, 2017
@MylesBorins
2017-06-06, Version 6.11.0 'Boron' (LTS)
4c5cbb7 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    #10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    #10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    #8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    #8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    #9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    #11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    #12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    #11094
  - upgrade libuv to 1.10.2 (cjihrig)
    #10717
  - upgrade libuv to 1.10.1 (cjihrig)
    #9647
  - upgrade libuv to 1.10.0 (cjihrig)
    #9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    #9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    #10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    #10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    #11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    #10294

PR-URL: #13059
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@mcollina @andrew749
crypto: make LazyTransform compabile with Streams1
4980c49 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs/node#12269
PR-URL: nodejs/node#12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@MylesBorins @andrew749
2017-06-06, Version 6.11.0 'Boron' (LTS)
e5cdc26 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    nodejs/node#10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    nodejs/node#10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    nodejs/node#8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    nodejs/node#8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    nodejs/node#9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    nodejs/node#11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    nodejs/node#12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    nodejs/node#11094
  - upgrade libuv to 1.10.2 (cjihrig)
    nodejs/node#10717
  - upgrade libuv to 1.10.1 (cjihrig)
    nodejs/node#9647
  - upgrade libuv to 1.10.0 (cjihrig)
    nodejs/node#9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    nodejs/node#9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    nodejs/node#10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    nodejs/node#2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    nodejs/node#10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    nodejs/node#11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    nodejs/node#10294

PR-URL: nodejs/node#13059
@akaila akaila mentioned this pull request Oct 19, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github left review comments

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. notable-change PRs with changes that should be highlighted in changelogs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mcollina @sam-github @MylesBorins @jasnell @addaleax @cjihrig @mscdex