Promisifying Crypto API #1
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boneskull
reviewed
Jun 4, 2020
boneskull
reviewed
Jun 4, 2020
| @@ -229,6 +229,8 @@ ObjectDefineProperty(constants, 'defaultCipherList', { | |||
| value: getOptionValue('--tls-cipher-list') | |||
| }); | |||
|
|
|||
| let promises; | |||
There was a problem hiding this comment.
In current master, the promises in fs are defined as:
let promises = null;then below in the get() method:
if (promises === null) {
// ..
}
boneskull
reviewed
Jun 4, 2020
| @@ -0,0 +1,3 @@ | |||
| exports = { | |||
boneskull
reviewed
Jun 4, 2020
| @@ -271,6 +271,16 @@ assert.throws( | |||
| } | |||
| ); | |||
|
|
|||
| assert.rejects( | |||
There was a problem hiding this comment.
seems good, but would expect at least a happy-path test. I assume that the promise implementations aren't generally tested as thoroughly as the callback implementations given they are usually just wrappers?
In the GitHub Actions CI, test-macos-app-sandbox.js can fail due to the application already being signed. This commit updates the test to handle that condition. Refs: nodejs#33944 PR-URL: nodejs#34331 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anto Aravinth <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
The messaging code uses `Object.defineProperty()`, which accesses `value` on `Object.prototype` by default, so some calls to the getter here would actually be expected. Instead, make the list of accessed properties more specific to the tested source map code to avoid flakiness. Refs: https://twitter.com/addaleax/status/1276289101671608320 Refs: nodejs#34057 PR-URL: nodejs#34446 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
This test has not been working correctly since at least a555be2. Since it tests internals, any replacement test might become invalid in a similar way. Refs: nodejs#34057 PR-URL: nodejs#34445 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Enable `NodeEventTarget` as a base class for `MessagePort`, by enabling special processing of events for Node.js listeners, and removing implicit constructors/private properties so that classes can be made subclasses of `NodeEventTarget` after they are created. PR-URL: nodejs#34057 Refs: https://twitter.com/addaleax/status/1276289101671608320 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Use `NodeEventTarget` to provide a mixed `EventEmitter`/`EventTarget` API interface. PR-URL: nodejs#34057 Refs: https://twitter.com/addaleax/status/1276289101671608320 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
For consistency with the rest of our docs and our style guide, use sentence-case rather than headline-case in the headers in quic.md. PR-URL: nodejs#34453 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Move benchmark CI to native suite since it requires building an addon. Refs: nodejs#34427 (comment) PR-URL: nodejs#34433 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34494 Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
The index ToC says "About these docs" but the document itself says "About this documentation" which I think is better. Use that. PR-URL: nodejs#34449 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Although most of the time openStream will be able to create the stream immediately, when a stream is opened before the handshake is complete we have to wait for the handshake to be complete before continuing. PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Removing no longer needed code PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
qlog files are diagnostic files that are being used to verify the quic implementation. Make sure they don't get checked in. PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
This one was a bit of a rabbit hole... but, with this set of changes, `QuicStream` should now work with autoDestroy, supports a promisified `close()`, and fixes a number of other internal bugs that were spotted trying to get it to work. PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34351 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34317 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Also fixes an internal cycle bug in the REPL exports resolution. PR-URL: nodejs#32261 Fixes: nodejs#31595 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Jan Krems <[email protected]>
This GitHub action takes quite a bit of time to build and is regularly flaky. Removing the OSX and Windows target from this action to avoid having red actions CI runs. Refs: nodejs#34123 PR-URL: nodejs#34440 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#34471 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
PR-URL: nodejs#34471 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
Remove checks in pummel/test-timers that are already checked in parallel/test-timers-clear-null-does-not-throw-error. PR-URL: nodejs#34473 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#34295 (comment) PR-URL: nodejs#34426 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Callout some practices explicitly, so that the process is followed in a similar manner PR-URL: nodejs#34455 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]>
PR-URL: nodejs#34782 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#34070 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
As best as I can tell, ERR_V8BREAKITERATOR is unused anywhere in our code base and dependencies. Move to legacy errors. PR-URL: nodejs#34792 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
In test-http-destroyed-socket-write2, the assert.strictEqual() in the default case of the switch statement will always fail because it checks for a value that is already accounted for in one of the switch cases. Convert it to assert.fail(). PR-URL: nodejs#34793 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
The text for the legacy API sends mixed signals. It's legacy, but still supported, so not deprecated, but not recommended. Let's begin to clarify this by removing "not recommended". If we want to not-recommend it, let's doc-deprecate it properly, or at least include an explanation as to why it's not recommended. PR-URL: nodejs#34697 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Documentation-only: Recommend people use the static methods on crypto.Certificate() and not the legacy API constructor. PR-URL: nodejs#34697 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Running outside of the main Node.js context prevents us from upgrading the WPT harness because new versions more aggressively check the identity of globals like error constructors. Instead of exposing globals used by the tests on vm sandboxes, use worker threads to run everything. PR-URL: nodejs#34796 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
There doesn't seem to be a reason for this test to have to stay in sequential. It appears to have been placed there out of caution. PR-URL: nodejs#34755 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#34800 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#34829 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
Refs: nodejs#34765 PR-URL: nodejs#34795 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#34786 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
In other api docs, it seems that deprecated classes and methods are listed along with others, and marked as deprecated. In tls docs, deprecations were listed at the bottom of the document. This commit reorders them to what seems to be the standard, and corrects some links in doc/api/deprecations.md PR-URL: nodejs#34687 Reviewed-By: Rich Trott <[email protected]>
Signed-off-by: Richard Lau <[email protected]> PR-URL: nodejs#34810 Refs: nodejs#34787 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Change format of logged messages when run on GitHub Actions (i.e. when the `GITHUB_ACTIONS` environment variable is true) so that broken links are highlighted inline in pull requests. Signed-off-by: Richard Lau <[email protected]> PR-URL: nodejs#34810 Refs: nodejs#34787 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Rich Trott <[email protected]>
The `module` core module is available for both CJS and ESM users, it deserves its own page. PR-URL: nodejs#34747 Refs: nodejs/modules#539 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Some stylistic changes to bring this more in line with what our tests currently look like, and add a note about general flakiness. PR-URL: nodejs#34685 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
This commit adds validation to the IP address returned by the net module's custom DNS lookup() function. PR-URL: nodejs#34813 Fixes: nodejs#34812 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#34584 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Jan Krems <[email protected]>
PR-URL: nodejs#34768 Fixes: nodejs#34767 Fixes: nodejs#34447 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Harshitha K P <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#34809 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Uses x64 node executable for running .js files in arm64 cross-compilation scenarios. MSI can now be created by running `vcbuild.bat release msi arm64` Refs: nodejs#25998 Refs: nodejs#32582 PR-URL: nodejs#34009 Reviewed-By: James M Snell <[email protected]> Reviewed-By: João Reis <[email protected]>
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: nodejs#34399 PR-URL: nodejs#34476 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
PR-URL: nodejs#34798 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Mary Marchini <[email protected]>
Keep references sorted in ASCII order in module.md. PR-URL: nodejs#34848 Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
Remove/add periods as appropriate in bulleted lists in BUILDING.md. PR-URL: nodejs#34849 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
test-cluster-net-listen-relative-path fails if run from the root directory on POSIX because the socket filename isn't quite long enough. Increase it by 2 so that the path length always exceeds 100 characters. PR-URL: nodejs#34820 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>
PR-URL: nodejs#34816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
joesepi
force-pushed
the
promisify-crypto
branch
from
a7f2c0f
to
1a034ff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes