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.

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

zlib: simplify flushing mechanism #23186

Closed
wants to merge 1 commit into from
Closed

Conversation

addaleax
Copy link
Member

Previously, flushing on zlib streams was implemented through stream 'drain' handlers. This has a number of downsides; in particular, it is complex, and could lead to unpredictable behaviour, since it meant that in a sequence like

compressor.write('abc');
compressor.flush();
waitForMoreDataAsynchronously(() => {
  compressor.write('def');
});

it was not fully deterministic whether the flush happens after the second chunk is written or the first one.

This commit replaces this mechanism by one that piggy-backs along the stream’s write queue, using a “special” Buffer instance that signals that a flush is currently due.

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

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Sep 30, 2018
@addaleax addaleax added the needs-citgm PRs that need a CITGM CI run. label Sep 30, 2018
@addaleax
Copy link
Member Author

addaleax commented Sep 30, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17538/ (:heavy_check_mark:)

I’d like to run CITGM on this once we’re back to a mostly normal state there – I’m not thinking this should be a breaking change, but I’d like to be extra-sure.

Edit: Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/241/

@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

Any other volunteers to review? :)

@addaleax
Copy link
Member Author

addaleax commented Oct 6, 2018

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-citgm PRs that need a CITGM CI run. labels Oct 6, 2018
Previously, flushing on zlib streams was implemented through
stream 'drain' handlers. This has a number of downsides; in
particular, it is complex, and could lead to unpredictable
behaviour, since it meant that in a sequence like

```js
compressor.write('abc');
compressor.flush();
waitForMoreDataAsynchronously(() => {
  compressor.write('def');
});
```

it was not fully deterministic whether the flush happens after
the second chunk is written or the first one.

This commit replaces this mechanism by one that piggy-backs
along the stream’s write queue, using a “special” `Buffer`
instance that signals that a flush is currently due.
@addaleax
Copy link
Member Author

addaleax commented Oct 7, 2018

CITGM results were full of compilation errors because I forgot to rebase…

Again: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1576/

@addaleax
Copy link
Member Author

addaleax commented Oct 9, 2018

Landed in e688fe6

@addaleax addaleax closed this Oct 9, 2018
@addaleax addaleax deleted the zlib-flush branch October 9, 2018 03:08
addaleax added a commit that referenced this pull request Oct 9, 2018
Previously, flushing on zlib streams was implemented through
stream 'drain' handlers. This has a number of downsides; in
particular, it is complex, and could lead to unpredictable
behaviour, since it meant that in a sequence like

```js
compressor.write('abc');
compressor.flush();
waitForMoreDataAsynchronously(() => {
  compressor.write('def');
});
```

it was not fully deterministic whether the flush happens after
the second chunk is written or the first one.

This commit replaces this mechanism by one that piggy-backs
along the stream’s write queue, using a “special” `Buffer`
instance that signals that a flush is currently due.

PR-URL: #23186
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 10, 2018
Previously, flushing on zlib streams was implemented through
stream 'drain' handlers. This has a number of downsides; in
particular, it is complex, and could lead to unpredictable
behaviour, since it meant that in a sequence like

```js
compressor.write('abc');
compressor.flush();
waitForMoreDataAsynchronously(() => {
  compressor.write('def');
});
```

it was not fully deterministic whether the flush happens after
the second chunk is written or the first one.

This commit replaces this mechanism by one that piggy-backs
along the stream’s write queue, using a “special” `Buffer`
instance that signals that a flush is currently due.

PR-URL: #23186
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Previously, flushing on zlib streams was implemented through
stream 'drain' handlers. This has a number of downsides; in
particular, it is complex, and could lead to unpredictable
behaviour, since it meant that in a sequence like

```js
compressor.write('abc');
compressor.flush();
waitForMoreDataAsynchronously(() => {
  compressor.write('def');
});
```

it was not fully deterministic whether the flush happens after
the second chunk is written or the first one.

This commit replaces this mechanism by one that piggy-backs
along the stream’s write queue, using a “special” `Buffer`
instance that signals that a flush is currently due.

PR-URL: #23186
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants