test: add tests for clearBuffer state machine #9922

captainsafia · 2016-12-01T16:32:43Z

Checklist

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

Affected core subsystem(s)

stream, test

Description of change

This checks to see that clearBuffer appropriately decrements the
correct values in _writableState when clearBuffer is invoked in
end.

Closes #8687.

bnoordhuis

Someone from @nodejs/streams should do the final sign-off but LGTM insofar I'm able to judge that.

CI: https://ci.nodejs.org/job/node-test-pull-request/5077/

bnoordhuis · 2016-12-01T21:30:52Z

test/parallel/test-stream-writableState-uncorked-bufferedRequestCount.js

+'use strict';
+
+const common = require('../common');
+


Tiny nit: no blank line here.

captainsafia · 2016-12-01T21:56:15Z

@bnoordhuis: Good catch on the blank line! Updated the code!

mcollina · 2016-12-01T23:30:27Z

test/parallel/test-stream-writableState-uncorked-bufferedRequestCount.js

+};
+
+writable._writableState.corked = 1;
+writable._writableState.bufferedRequestCount = 1;


can you please trigger the corked and bufferedRequestCount things using the normal API?

captainsafia · 2016-12-02T01:58:37Z

@mcollina: Thanks for the comment! I've tried to address your review.

I set corked use the cork function but I'm having a little bit of trouble setting the bufferedRequestCount using the API. From what I can say, it is incremented in writeOrBuffer which is called in write if the chunk provided is a valid chunk. For some strange reason, this bit of code doesn't produce the desired effect (incrementing bufferedRequestCount by 1 as done in writeOrBuffer).

writable.write('some-stuff', 'utf8', common.mustCall(() => {
    assert.strictEqual(writable._writableState.bufferedRequestCount, 1);
}));

I'm trying to trace what's going on but before I invest too much time I figured I might ask you and see if I am missing an obvious way to do this.

mcollina · 2016-12-02T21:35:29Z

@captainsafia you need to have an async _write, and have write() being issued. Another alternative is to use cork() and uncork(). Let me know if you have any more questions, happy to chat on IRC if needed.

captainsafia · 2016-12-03T06:59:21Z

@mcollina: Thanks for the guidance! I'll try pursuing it on my own for a bit next week and get in touch with you on IRC if I'm not getting anywhere.

Trott · 2016-12-11T06:18:54Z

Bump! @captainsafia Are you still working on this?

captainsafia · 2016-12-11T06:28:07Z

Yep! I did some tinkering with it yesterday but I don't think I have anything commit worthy. I'll see if I can get something tomorrow or Monday. Sorry for leaving this hanging!

captainsafia · 2016-12-13T22:24:07Z

@mcollina: Couldn't catch you on IRC so here goes.

@captainsafia you need to have an async _write, and have write() being issued.

I've got a barebones _write = (chunk, encoding, cb) implemented in the tests here. Should write be issue inside that function...or?

Another alternative is to use cork() and uncork().

I'm using cork to update the corked state right now but from what I can tell it doesn't change bufferedRequestCount so it's seems like not a complete alternative.

Fishrock123 · 2016-12-13T22:32:11Z

test/parallel/test-stream-writableState-uncorked-bufferedRequestCount.js

+
+const writable = new stream.Writable();
+
+writable._write = (chunk, encoding, cb) => {


We should consider using a regular function here, as we make no guarentees that apply() or call() may not be called on this internally in the future.

(Assigning arrow functions as prototype functions is generally bad practice because those do not work.)

mcollina · 2016-12-14T11:54:56Z

bufferedRequestCount is incremented when a write is in progress, or if the stream is corked:

node/lib/_stream_writable.js

Lines 310 to 318 in f3cf8e9

    
           if (state.writing || state.corked) { 
        
             var last = state.lastBufferedRequest; 
        
             state.lastBufferedRequest = new WriteReq(chunk, encoding, cb); 
        
             if (last) { 
        
               last.next = state.lastBufferedRequest; 
        
             } else { 
        
               state.bufferedRequest = state.lastBufferedRequest; 
        
             } 
        
             state.bufferedRequestCount += 1;

.

You should issue multiple writes synchronously after cork(), check bufferedRequestCount and then uncork().

Note that bufferedRequestCount makes most sense if _writev is implemented, see

node/lib/_stream_writable.js

Line 409 in f3cf8e9

var l = state.bufferedRequestCount;

. You should probably double the checks, one for a stream just with _write, and one with a stream with _writev.

Trott · 2016-12-21T23:26:20Z

Ping @captainsafia! Still working on this? (No rush; I'm just making sure it's not stalled.)

captainsafia · 2016-12-23T03:42:36Z

Sorry for staying delayed on this. I'd love to see this own through but I think it is a tad bit above my pay grade at the moment so I'll close it and give someone else the opportunity to tackle it with some of the insights provided above.

mcollina · 2016-12-23T07:32:33Z

@captainsafia may I build upon your commit?

captainsafia · 2016-12-23T17:05:58Z

@mcollina: Sure! I've added you as a collaborator on my fork.

mcollina · 2017-01-02T12:28:22Z

@captainsafia I force pushed this. Let me know if you are ok with the content.

This checks to see that clearBuffer appropriately decrements the correct values in _writableState when clearBuffer is invoked in end. Fixes: nodejs#8687 PR-URL: nodejs#9922 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mcollina · 2017-01-02T17:02:41Z

@italoacasas should be addressed.

italoacasas · 2017-01-02T17:35:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5664/console

Trott · 2017-01-02T20:03:13Z

CI failures on AIX and FreeBSD seem unrelated. I just terminated some stalled tests on those machines--probably a previous test run had a bug that caused cluster tests to hang and not free ports, causing subsequent runs like this one to also fail.

Re-running CI: https://ci.nodejs.org/job/node-test-pull-request/5665/

italoacasas · 2017-01-03T12:46:55Z

Landed 275362a

captainsafia · 2017-01-03T14:19:13Z

Thanks for getting this through the finish line, everyone! ❤️

This checks to see that clearBuffer appropriately decrements the correct values in _writableState when clearBuffer is invoked in end. Fixes: #8687 PR-URL: #9922 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016

mscdex added the stream Issues and PRs related to the stream subsystem. label Dec 1, 2016

imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016

bnoordhuis approved these changes Dec 1, 2016

View reviewed changes

captainsafia force-pushed the add-writable-state-test branch from 121b512 to 6c33afe Compare December 1, 2016 21:55

mcollina requested changes Dec 1, 2016

View reviewed changes

Fishrock123 reviewed Dec 13, 2016

View reviewed changes

captainsafia closed this Dec 23, 2016

captainsafia reopened this Dec 23, 2016

mcollina force-pushed the add-writable-state-test branch from 59f370f to 9d1d739 Compare January 2, 2017 12:27

nodejs-github-bot added lts-watch-v6.x labels Jan 2, 2017

mcollina requested a review from italoacasas January 2, 2017 12:28

mcollina approved these changes Jan 2, 2017

View reviewed changes

mcollina force-pushed the add-writable-state-test branch from 9d1d739 to 53cf711 Compare January 2, 2017 13:49

mcollina force-pushed the add-writable-state-test branch from 53cf711 to d1709c3 Compare January 2, 2017 16:56

nodejs-github-bot added lts-watch-v6.x labels Jan 2, 2017

test: add tests for clearBuffer state machine

b0be2ac

This checks to see that clearBuffer appropriately decrements the correct values in _writableState when clearBuffer is invoked in end. Fixes: nodejs#8687 PR-URL: nodejs#9922 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mcollina force-pushed the add-writable-state-test branch from d1709c3 to b0be2ac Compare January 2, 2017 17:02

nodejs-github-bot added lts-watch-v6.x labels Jan 2, 2017

italoacasas approved these changes Jan 2, 2017

View reviewed changes

italoacasas closed this Jan 3, 2017

evanlucas mentioned this pull request Jan 4, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add tests for clearBuffer state machine #9922

test: add tests for clearBuffer state machine #9922

captainsafia commented Dec 1, 2016

bnoordhuis left a comment

bnoordhuis Dec 1, 2016

captainsafia commented Dec 1, 2016

mcollina Dec 1, 2016

captainsafia commented Dec 2, 2016

mcollina commented Dec 2, 2016

captainsafia commented Dec 3, 2016

Trott commented Dec 11, 2016

captainsafia commented Dec 11, 2016

captainsafia commented Dec 13, 2016

Fishrock123 Dec 13, 2016 •

edited

Loading

mcollina commented Dec 14, 2016 •

edited

Loading

Trott commented Dec 21, 2016

captainsafia commented Dec 23, 2016

mcollina commented Dec 23, 2016

captainsafia commented Dec 23, 2016

mcollina commented Jan 2, 2017

mcollina commented Jan 2, 2017

italoacasas commented Jan 2, 2017

Trott commented Jan 2, 2017

italoacasas commented Jan 3, 2017

captainsafia commented Jan 3, 2017


		const writable = new stream.Writable();

		writable._write = (chunk, encoding, cb) => {

test: add tests for clearBuffer state machine #9922

test: add tests for clearBuffer state machine #9922

Conversation

captainsafia commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 1, 2016

Choose a reason for hiding this comment

captainsafia commented Dec 1, 2016

mcollina Dec 1, 2016

Choose a reason for hiding this comment

captainsafia commented Dec 2, 2016

mcollina commented Dec 2, 2016

captainsafia commented Dec 3, 2016

Trott commented Dec 11, 2016

captainsafia commented Dec 11, 2016

captainsafia commented Dec 13, 2016

Fishrock123 Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

mcollina commented Dec 14, 2016 • edited Loading

Trott commented Dec 21, 2016

captainsafia commented Dec 23, 2016

mcollina commented Dec 23, 2016

captainsafia commented Dec 23, 2016

mcollina commented Jan 2, 2017

mcollina commented Jan 2, 2017

italoacasas commented Jan 2, 2017

Trott commented Jan 2, 2017

italoacasas commented Jan 3, 2017

captainsafia commented Jan 3, 2017

Fishrock123 Dec 13, 2016 •

edited

Loading

mcollina commented Dec 14, 2016 •

edited

Loading