stream: WritableBase without buffering #16
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
ronag
force-pushed
the
writable-base
branch
9 times, most recently
from
cd7851e
to
b1c4993
Compare
ronag
force-pushed
the
writable-base
branch
2 times, most recently
from
c83723e
to
a3fdb90
Compare
PR-URL: nodejs#33717 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#33913 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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 <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#29829 PR-URL: nodejs#33161 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
PR-URL: nodejs#33804 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This commit uses Maybe::Check() instead of Maybe::FromJust() as the return value is not used. PR-URL: nodejs#33909 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#33919 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
PR-URL: nodejs#33863 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
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 <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
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 <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Using the new experimental AbortController... Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#33833 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
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 <[email protected]> PR-URL: nodejs#33912 Reviewed-By: David Carlier <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
PR-URL: nodejs#33775 Fixes: nodejs#33773 Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#33715 PR-URL: nodejs#33765 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#33715 PR-URL: nodejs#33767 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-by: Sathya Gunasekaran <[email protected]>
Commit-Queue: Gus Caplan <[email protected]>
Cr-Commit-Position: refs/heads/master@{#68207}
Refs: v8/v8@eec10a2
PR-URL: nodejs#33778
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]>
PR-URL: nodejs#33926 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#22413 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
PR-URL: nodejs#32758 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use a common function to handle alignment computations in multiple places. PR-URL: nodejs#33884 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
- 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> PR-URL: nodejs#33797 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: nodejs#33801 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
Signed-off-by: Michaël Zasso <[email protected]> PR-URL: nodejs#33857 Refs: nodejs#33770 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
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 <[email protected]>
Reviewed-by: Jakob Gruber <[email protected]>
Cr-Commit-Position: refs/heads/master@{#67630}
Refs: v8/v8@767e65f
PR-URL: nodejs#33859
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#33859 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Fixes: nodejs#28001 PR-URL: nodejs#33911 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
- 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 <[email protected]> Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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
Writableclass into two classes aWritableBaseclass which implements allWritablelogic except for anything related to buffering, and aWritableclass which inheritsWritableBaseand adds the buffering logic.This is in order to be able to consistently and in a performant way implement
Writablestreams that want to implement their own buffering or are simply wrapping existing streams.WritableBaseexpects 2 methods passed throughoptions:write: which is basically thewriteOrBufferimplementation 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
Writablelike 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
Transformstream by implementing it in terms ofReadable+WritableBaseinstead ofReadable+Writable.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes