forked from nodejs/node
-
Notifications
You must be signed in to change notification settings - Fork 1
stream: WritableBase without buffering #16
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
Open
ronag
wants to merge
43
commits into
master
Choose a base branch
from
writable-base
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or 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
cd7851e
to
b1c4993
Compare
c83723e
to
a3fdb90
Compare
PR-URL: nodejs#33717 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This PR defines two new modes for the --unhandled-rejections flag. The first mode is called "throw". The "throw" mode first emits unhandledRejection. If this hook is not set, the "throw" mode will raise the unhandled rejection as an uncaught exception. The second mode is called "warn-with-error-code". The "warn-with-error-code" mode first emits unhandledRejection. If this hook is not set, the "warn-with-error-code" mode will trigger a warning and set the process's exit code to 1. The PR doesn't change the default behavior for unhandled rejections. That will come in a separate PR. Refs: nodejs#33021 PR-URL: nodejs#33475 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#33913 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
The stream doc has the only instance in our docs where two events are combined into a single entry. Split them into separate adjacent entries. PR-URL: nodejs#33881 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#29829 PR-URL: nodejs#33161 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
PR-URL: nodejs#33804 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Notable changes: deps: * V8: cherry-pick 548f6c81d424 (Dominykas Blyžė) [nodejs#33484](nodejs#33484) * update to uvwasi 0.0.9 (Colin Ihrig) [nodejs#33445](nodejs#33445) * upgrade to libuv 1.38.0 (Colin Ihrig) [nodejs#33446](nodejs#33446) * upgrade npm to 6.14.5 (Ruy Adorno) [nodejs#33239](nodejs#33239) PR-URL: nodejs#33811
Take ownership of the token value, since the memory for it is allocated anyway and the buffer size is just 16, i.e. copyable very cheaply. This makes valgrind stop complaining about a use-after-free error when running `sequential/test-quic-preferred-address-ipv6`. PR-URL: nodejs#33917 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This commit uses Maybe::Check() instead of Maybe::FromJust() as the return value is not used. PR-URL: nodejs#33909 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#33919 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
PR-URL: nodejs#33863 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Use `bash` instead of `shell` for code language flag in doc/guides/maintaining-ngtcp2-nghttp3.md to conform with our other docs and upcoming lint requirements. PR-URL: nodejs#33852 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
This adds linting for code fence language/grammar strings. This is so, for example, we have only one of ```text and ```txt and not both. PR-URL: nodejs#33852 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Using the new experimental AbortController... Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: nodejs#33833 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
this.unidirectional depends on #id which is never set in the constructor, hence this condition will never run and can be removed. PR-URL: nodejs#33914 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Original PR: nodejs/quic#388 Previously, QuicPacket was allocating an std::vector<uint8_t> of NGTCP2_MAX_PKT_SIZE bytes, then the packet would be serialized into the buffer, and the std::vector would be resized based on the number of bytes serialized. I suspect the memory fragmentation that you're seeing is because of those resize operations not freeing memory in chunks that are aligned with the allocation. This changes QuicPacket to use a stack allocation that is always NGTCP2_MAX_PKT_SIZE bytes and the size of the serialized packet is just recorded without any resizing. When the memory is freed now, it should be freed in large enough chunks to cover subsequent allocations. Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: nodejs#33912 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
PR-URL: nodejs#33775 Fixes: nodejs#33773 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#33715 PR-URL: nodejs#33765 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#33715 PR-URL: nodejs#33767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Original commit message: [promisehook] Add before/after hooks to thenable tasks This will allow Node.js to properly track async context in thenables. Change-Id: If441423789a78307a57ad7e645daabf551cddb57 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624 Reviewed-by: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org> Commit-Queue: Gus Caplan <me@gus.host> Cr-Commit-Position: refs/heads/master@{#68207} Refs: v8/v8@eec10a2 PR-URL: nodejs#33778 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit removes the unused <array> and <limits> includes, and adds <memory> (for std::unique_ptr), and <utility> (for std::move). PR-URL: nodejs#33921 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#33926 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#22413 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
PR-URL: nodejs#32758 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>
PR-URL: nodejs#33889 Refs: https://developers.google.com/style/capitalization#capitalization-in-titles-and-headings Refs: https://docs.microsoft.com/en-us/style-guide/capitalization Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously, our documentation headers were a mixture of title case, sentence case, and things that were neither. For technical documentation, the _de facto_ standard seems to be sentence case. (See refs below.) So let's standardize on that. This commit follows a commit implementing this standard. This commit adds it to the style guide. Refs: https://developers.google.com/style/capitalization#capitalization-in-titles-and-headings Refs: https://docs.microsoft.com/en-us/style-guide/capitalization PR-URL: nodejs#33889 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Use a common function to handle alignment computations in multiple places. PR-URL: nodejs#33884 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
- Don’t use 12 as a magic number for the buffer size - Mark the object as weak (which is conceptually the right thing to do, even if there is little practical impact) - Keep a reference to the `ArrayBuffer` in question for memory tracking PR-URL: nodejs#33851 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This makes sure colors that are unknown won't cause an error. This is especially important in case a library wants to use colors defined by Node.js core, if available and fall back to the default otherwise. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: nodejs#33797 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: nodejs#33801 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
With the async_hooks callback trampoline, domains no longer need any native code. With this, domains can exist in pure JavaScript. PR-URL: nodejs#33801 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Signed-off-by: Michaël Zasso <targos@protonmail.com> PR-URL: nodejs#33857 Refs: nodejs#33770 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Original commit message: [API] Fix microtask message reporting RunSingleMicrotask calls Runtime::ReportMessage, but the implementation of ReportMessage would unconditionally discard these exceptions. This CL removes all of the intermediate logic and directly calls MessageHandler::ReportMessage, restoring the ability of RunSingleMicrotask to report exceptions that occur in microtasks. Bug: v8:8326 Change-Id: I493de74383b2ab191d786611fb9eba9d27e7a243 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162121 Commit-Queue: Gus Caplan <me@gus.host> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#67630} Refs: v8/v8@767e65f PR-URL: nodejs#33859 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#33859 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
When importing specific names from a CJS module, and renaming them using `as`, the example fix in the error message erroneously contains the keyword `as` in the destructuring variable declaration. Example of this issue: import { parse as acornParse } from "acorn"; ^^^^^ SyntaxError: The requested module 'acorn' is expected to be of type CommonJS, which does not support named exports. CommonJS modules can be imported by importing the default export. For example: import pkg from 'acorn'; const { parse as acornParse } = pkg; PR-URL: nodejs#33882 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Fixes: nodejs#28001 PR-URL: nodejs#33911 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
- Code sample updated by adding a hello-world (`demo.wat`) code example - Step for compiling `.wat` to `.wasm` added (with reference to `wabt` tools) - The sample code prints "hello world\n" in the console This update adds a very minimal change to the existing sample and can be treated as an extension. PR-URL: nodejs#33626 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
The [Contributor's Survey results](nodejs/TSC#882) highlight the fact that it is often not easy for contributors to know who the right people are to talk to about a proposed change or who to ask for reviews of a given subsystem. We briefly toyed around with codeowners before when GitHub introduced it but just as quickly disabled it because the structure of our repository made it exceedingly difficult to get right. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: nodejs#33895 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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.
This separates the
Writable
class into two classes aWritableBase
class which implements allWritable
logic except for anything related to buffering, and aWritable
class which inheritsWritableBase
and adds the buffering logic.This is in order to be able to consistently and in a performant way implement
Writable
streams that want to implement their own buffering or are simply wrapping existing streams.WritableBase
expects 2 methods passed throughoptions
:write
: which is basically thewriteOrBuffer
implementation from today.flush
: which is basicallyend()
+ wait for completion from today.Note, that this is for now only for internal core usage not meant for public API.
This will help with making
Writable
like implementations where we want to avoid buffering more streams compliant, e.g.OutgoingMessage
.It will also help with some optimization e.g. avoid buffering both input and output of
Transform
stream by implementing it in terms ofReadable
+WritableBase
instead ofReadable
+Writable
.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes