meta: planning for promisified fs and crypto #15413
Comments
Is there any reassuring info about it? They are not mentioned in the official blog for 6.2 and still under the flag in the 6.3 dev. |
|
It may come later. If it does those pieces will wait or I'll take a more iterative approach |
|
Sounds awesome! Thanks James. One question I do have... for async crypto, will we be adding actual async apis or just wrapping them in a promise (since most of them are really just sync streams with the exception of those with *Sync counterparts)? |
|
I'm planning to work on actual async crypto. This will mean making fairly large updates to the native layer, so it will take some time. |
mscdex
added
crypto
|
@jasnell when you say async crypto - what do you actually mean? Most of the crypto is too fast for being threaded, and things that are slow (like DH) are already offloaded to the worker threads. I'm all in for promisified crypto, but it might be as well done just using |
|
Yeah, I played around with this quite some time ago also and it's going to take some doing and I do not really anticipate performance being the critical success factor on it. The idea more is to support the ergonomics of the Promises use case. @indutny ... I'm going to be experimenting with different options here and benchmarking the heck out of everything as I go. For most things, we ought to be able to just wrap the existing sync APIs into a promise and use async iterators. For the most part, that will Just Work(tm) for the majority of use cases. For the most part, however, I'm still working out exactly what it'll mean and I should warn you to expect many pings from me as I go through it :-) |
|
Here's an example of what I mean about wrapping things into a Promise and using async iterators... 'use strict';
const crypto = require('crypto');
async function hash(alg, data, enc) {
const hash = crypto.createHash(alg);
for await (const chunk of data) {
hash.update(chunk);
}
return hash.digest(enc);
}
var n = 0;
async function* dataIterator() {
while (n++ < 10)
yield `testing${n}`;
}
const p = hash('sha256', dataIterator(), 'hex');
p.then(console.log).catch(console.error);The crypto operation itself is still sync... |
|
That looks good. My point was that moving it to threads might be slower than doing it in main thread. |
|
yep, I think for the overwhelming majority of situations that's definitely going to be the case |
Leaving My suggestion would be to take advantage of the fact that Modules are still experimental and are expected to break some things. I made an informal proposal in nodejs/CTC#12 (comment) but it seems it didn't get much attention, so I'll take the liberty of quoting it here.
|
|
@seishun I think what you propose would be confusing in practice. I don't like the idea of having I'd be fine with importing/requiring |
|
If changing the 'crypto' API to be promise based it should really move to be aligned with Web Crypto. It is not a perfect API but being able to share code across browser and server is very powerful. Two projects I've created in this spirit are: |
|
That's moving the goalposts too much, IMO. One thing at a time. |
I'd personally be fine with that, I just suggested an alternative that doesn't require that. @jasnell perhaps you could conduct another Twitter poll to find out how people feel about |
|
I've started working on the implementation for Note that the traditional async method is consistently much faster and that using |
|
@jasnell Hey, this reminded me of something :) One thing that I had in mind when I wrote the code for The reason I haven’t implemented that right away is that it was blocked #5691 (the microtask queue would not be run after the resolution), plus it would have broken async_hooks tracking. However, since we just fixed that by adding |
|
Great minds and something ;-) ... yeah that's right along the lines of what I was thinking but I'm going to be playing with a few ideas along the way. (specifically, I'm going to see about creating the Promise in native-land without having to set the |
|
@targos has a branch that allowed the use of more efficient promise operations in JS through V8 Extras. Since Promise creation and resolution are not actually implemented in C++ but rather into TurboFan's Some preliminary results from that branch against /cc @nodejs/v8 More context@littledan said:
@gsathya said:
The node/deps/v8/src/builtins/builtins-promise-gen.cc Lines 1204 to 1212 in 631c59b node/deps/v8/src/bootstrapper.cc Lines 2226 to 2232 in 631c59b node/deps/v8/src/bootstrapper.cc Lines 4322 to 4323 in 631c59b
Lines 7566 to 7570 in 631c59b Similarly for promise creation: node/deps/v8/src/builtins/builtins-promise-gen.cc Lines 1152 to 1156 in 631c59b node/deps/v8/src/bootstrapper.cc Lines 2211 to 2218 in 631c59b node/deps/v8/src/bootstrapper.cc Lines 4320 to 4321 in 631c59b Lines 7535 to 7544 in 631c59b |
|
@TimothyGu I know that makes sense when promisifying userland code, but does it matter as much even when we’re calling from C++ code anyway whether we do |
|
I'll have some benchmark results on the resolve from C++ either today or Monday. |
|
I'm in conferences and trips until Oct 12th but afternoon that I want to schedule the promise performance meeting so we can move ahead with performance |
|
Updated the code to resolve from C/C++, still not the most efficient iteration but definitely an improvement |
|
@jasnell just curious, would you be willing to share the code/benchmarks used for that? |
|
Yeah, will likely push the branch in the morning. I'm still actively iterating on it. Not really in Write-Code-That-I'd-Actually-Push mode yet, just playing with variations. Just made another tweak and the results are much better... |
In that case, probably not. |
|
Playing around with it, @addaleax is right, it doesn't make much difference using the v8-extras. For now, I'm going with the method she suggested and I'm working on a number of other improvements along the way. The PR is going to end up being semver-major because it's going to include some needed improvements to error handling in the existing fs code, in addition to switching to internal/errors and adding the promise versions. I'm expecting a fairly large PR. |
|
Should have posted #15204 (comment) here: WHATWG landed const controller = new AbortController();
const signal = controller.signal;
setTimeout(() => controller.abort(), 5000);
fetch(url, { signal }).then(response => {
return response.text();
}).then(text => {
console.log(text);
}).catch(err => {
if (err.name === 'AbortError') {
console.log('Fetch aborted');
} else {
console.error('Uh oh, an error!', err);
}
});We could implement that (even before promisifying the API) as a means to cancel ongoing async tasks. P.S. |
|
@refack |
|
If there will be a way to request the promisified version of fs, how would this deal with situations where a module becomes promisified in the future? What I mean, if I request the promisified version of fs, createWriteStream would not be promise capable - but I posted #16989 intended to make promisify support that function. So would |
|
@coreyfarrell Why would you even bother bringing this up. This makes no sense. Promisifying |
|
@MikeKovarik Please try to stay respectful of each other on this issue tracker.
Also, we don’t quite have a good story for mixing streams with promises yet anyway. |
|
+1 to what @addaleax said. There are many considerations here and much we have yet to work out. I will be updating the initial draft of the fs promises impl in the coming couple of weeks. |
|
@MikeKovarik: I only commented here so everyone can consider how |
|
@addaleax Yes, I crossed the line there, I'm sorry. @coreyfarrell What drove me over the edge on this topic is the ammount of discussion on this topic (or if it should be done at all which we're thanfully over now given that this issue exists). I as an every day Node user see this as "Why is there even a discussion when there's obvious solution at hand". This reminds me of the same thing I've seen with Angular vs Aurelia framework. Back when Angular 2 was being developed, the devs kept arguing and making decisions to prioritize performance as much as possible which striped it of all the things that made Angular 1 so popular - two way binding. That's why Rob Eisenberg went his own way to create Aurelia. He and his team somewhere stated that "they're not a framework builders, they are app builders first" and that's what makes Aurelia's API so much more pleasing to use than Angular. What I (and I believe a lot of node users) would like to see from promisified I do care about Node, because I love using it. I would like this to go through as an exciting bullet in changelog, rather than something that just brings confusion. |
|
Thanks for owning up @MikeKovarik - I'll try explaining why things are happening so slowly.
It took me very long to not have this mindset, it is very tempting to think that solving this is easy but this isn't easy from an API perspective and it isn't easy from a "what works" perspective. There have been previous attempts before. There is a lot of information there about omitting the last argument vs. other approaches (mostly, some methods don't allow this, some are not well behaved - that sort of thing). As a platform, Node has to be very careful about what we break, even in semver-major, we've made this mistake before with breaking tools which cost a lot of people a lot of time debugging. This is why Node.js has a "canary in the goldmine" process - you can see this talk for more information about it. Whenever Node.js breaks something - the community trusts Node.js a little less.
We all do, and we're making progress - it's just a lot of work and the project is community driven mostly, adding I encourage you to try participating more in the project - we're always looking for volunteers to help maintain the project and promote new features. If you'd like to get more involved feel free to reach out to me (my email is in the home page) and I'll try to work with you - there are several issues that are suitable for new contributors relating to promises :) Sorry for the off topic, I thought this deserves a public reply for future readers. |
|
@benjamingr Thanks for the explainer. I understand breaking changes are difficult. But I'm worried that if we stop breaking things from time to time, we will eventually stop innovating.
I would love to. As a matter of fact I was considering contributing recently. Over the past couple of weeks I was going over the Also sorry for going OP. I thought I'd drop in a plug for uwp-fs :) since I've already written it with promises and async/await. |
|
@MikeKovarik ... again at the risk of going off topic, my advice would be to start small, focusing on focused incremental improvements then working up from there. I do I a work in progress PR and a larger effort underway to add promises to the |
|
Just going back a little bit in this thread to the merits of doing async crypto in the threadpool: It all depends on buffer size. Buffer size is critical when talking about async crypto performance. Crypto throughput and latency on the main thread is better for buffers less than 1024 bytes, but anything larger is probably better off in the threadpool for a miniscule latency loss but much higher throughput overall. Here are some actual numbers for 1 MB buffers, see crypto-async: If Node can only ever do single core crypto, then Node will never be able to do high-throughput crypto or compete with systems that can. |
|
@jasnell - what would be the next action on this? Has the discussion run its course to be able to take any decision? |
|
Step one was getting fs/promises delivered and ensuring it was stable. Next is identifying the other modules that make sense to give similar treatment to and setting up a tracking issue to monitor progress. Promisified crypto is going to be a challenge because it's going to be difficult to do in any performant way. We can use this as the tracking issue or close it and open a separate one. Whichever works best |
|
@jasnell - given lot of valuable insights above, I don't feel like proposing a close. However, as a prospect of a PR is not around, the issue just lingers, so I leave it upto you for a call! |
|
A |
|
Is it possible to consider supporting // Direct import
import {readFile} from 'fs/promises';
// Current method
import fs from 'fs';
const {readFile} = fs.promises;I'm not necessarily against the existence of |
|
Hey @coreyfarrell thanks for chiming in. It was |
|
@benjamingr Thanks for the response and link. This addresses my concern as it seems a long-term plan is being developed to provide direct import of the fs promises members. |
|
@coreyfarrell there is, yeah. |
|
We can close this tracking issue now that the work is progressing |
This is largely a heads up.
Yesterday I posted a poll to Twitter asking how users felt about core providing a promisified API for
fs... with two hours remaining in the poll, this is the result:It's clear that there is strong demand. I have heard from many who like the fact that
util.promisify()is now a thing but feel that being forced to use it in order to get promisified core APIs is not a very ergonomic experience, and while many are using Promise-wrapper libraries to achieve this, there is obviously a strong sentiment towards having these provided by core itself.Therefore, I will be actively working towards the goal of providing Promise-enabled versions of all of the
fsandcryptoAPIs in core. This will not be done all at once as there are a number of changes that may be required. For the crypto APIs, I will likely wait until async-iterators have landed with V8 6.2.I understand that not everyone is in love with Promises and not everyone wants to use them. The changes I have in mind will not touch the existing APIs, so anyone who wants to ignore the Promisified versions will be able to.
The text was updated successfully, but these errors were encountered: