Skip to content

Commit

Permalink
Remove ReadableStream.prototype's closed getter
Browse files Browse the repository at this point in the history
Per discussion in #297. We can always add it back later if we really want it, but it has become increasingly useless in the newer, reader-centric design.
  • Loading branch information
domenic committed Mar 16, 2015
1 parent 36dbd70 commit 1bfbd5e
Show file tree
Hide file tree
Showing 19 changed files with 79 additions and 203 deletions.
21 changes: 2 additions & 19 deletions index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,6 @@ would look like
class ReadableStream {
constructor(underlyingSource = {})

get closed()

cancel(reason)
getReader()
pipeThrough({ writable, readable }, options)
Expand Down Expand Up @@ -353,7 +351,6 @@ Instances of <code>ReadableStream</code> are created with the internal slots des

<ol>
<li> Set <b>this</b>@\[[underlyingSource]] to <var>underlyingSource</var>.
<li> Set <b>this</b>@\[[closedPromise]] to a new promise.
<li> Set <b>this</b>@\[[queue]] to a new empty List.
<li> Set <b>this</b>@\[[state]] to <code>"readable"</code>.
<li> Set <b>this</b>@\[[started]], <b>this</b>@\[[draining]], and <b>this</b>@\[[pullScheduled]] to <b>false</b>.
Expand All @@ -379,18 +376,6 @@ Instances of <code>ReadableStream</code> are created with the internal slots des

<h4 id="rs-prototype">Properties of the <code>ReadableStream</code> Prototype</h4>

<h5 id="rs-closed">get closed</h5>

<div class="note">
The <code>closed</code> getter returns a promise that will be fulfilled when the stream becomes closed, or rejected
if it ever errors.
</div>

<ol>
<li> If IsReadableStream(<b>this</b>) is <b>false</b>, return a promise rejected with a <b>TypeError</b> exception.
<li> Return <b>this</b>@\[[closedPromise]].
</ol>

<h5 id="rs-cancel">cancel(reason)</h5>

<div class="note">
Expand Down Expand Up @@ -585,8 +570,8 @@ Instances of <code>ReadableStreamReader</code> are created with the internal slo
<h5 id="reader-closed">get closed</h5>

<div class="note">
While the reader is <a lt="active reader">active</a>, the promise returned by the <code>closed</code> getter for a
stream reader will behave the same as that for the original stream, for convenience.
The <code>closed</code> getter returns a promise that will be fulfilled when the stream becomes closed or the
reader's lock is <a lt="release a read lock">released</a>, or rejected if the stream ever errors.
</div>

<ol>
Expand Down Expand Up @@ -744,7 +729,6 @@ Instances of <code>ReadableStreamReader</code> are created with the internal slo

<ol>
<li> Assert: <var>stream</var>@\[[state]] is <code>"readable"</code>.
<li> Resolve <var>stream</var>@\[[closedPromise]] with <b>undefined</b>.
<li> Set <var>stream</var>@\[[state]] to <code>"closed"</code>.
<li> If IsReadableStreamLocked(<var>stream</var>) is <b>true</b>, return
ReleaseReadableStreamReader(<var>stream</var>).
Expand Down Expand Up @@ -848,7 +832,6 @@ a variable <var>stream</var>, that performs the following steps:
<ol>
<li> If <var>stream</var>@\[[state]] is not <code>"readable"</code> return <b>undefined</b>.
<li> Let <var>stream</var>@\[[queue]] be a new empty List.
<li> Reject <var>stream</var>@\[[closedPromise]] with <var>e</var>.
<li> Set <var>stream</var>@\[[storedError]] to <var>e</var>.
<li> Set <var>stream</var>@\[[state]] to <code>"errored"</code>.
<li> If IsReadableStreamLocked(<var>stream</var>) is <b>true</b>,
Expand Down
41 changes: 3 additions & 38 deletions reference-implementation/lib/readable-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { DequeueValue, EnqueueValueWithSize, GetTotalQueueSize } from './queue-w
export default class ReadableStream {
constructor(underlyingSource = {}) {
this._underlyingSource = underlyingSource;
this._initClosedPromise();
this._queue = [];
this._state = 'readable';
this._started = false;
Expand All @@ -29,14 +28,6 @@ export default class ReadableStream {
);
}

get closed() {
if (IsReadableStream(this) === false) {
return Promise.reject(new TypeError('ReadableStream.prototype.closed can only be used on a ReadableStream'));
}

return this._closedPromise;
}

cancel(reason) {
if (IsReadableStream(this) === false) {
return Promise.reject(new TypeError('ReadableStream.prototype.cancel can only be used on a ReadableStream'));
Expand Down Expand Up @@ -81,7 +72,7 @@ export default class ReadableStream {

reader = source.getReader();

source.closed.catch(abortDest);
reader.closed.catch(abortDest);
dest.closed.then(
() => {
if (!closedPurposefully) {
Expand All @@ -105,7 +96,7 @@ export default class ReadableStream {
}
});

// Any failures will be handled by listening to source.closed and dest.closed above.
// Any failures will be handled by listening to reader.closed and dest.closed above.
// TODO: handle malicious dest.write/dest.close?
}

Expand Down Expand Up @@ -150,31 +141,7 @@ export default class ReadableStream {
rejectPipeToPromise(reason);
}
}


// Note: The resolve function and reject function are cleared when the corresponding promise is resolved or rejected.
// This is for debugging. This makes extra resolve/reject calls for the same promise fail so that we can detect
// unexpected extra resolve/reject calls that may be caused by bugs in the algorithm.

_initClosedPromise() {
this._closedPromise = new Promise((resolve, reject) => {
this._closedPromise_resolve = resolve;
this._closedPromise_reject = reject;
});
}

_resolveClosedPromise(value) {
this._closedPromise_resolve(value);
this._closedPromise_resolve = null;
this._closedPromise_reject = null;
}

_rejectClosedPromise(reason) {
this._closedPromise_reject(reason);
this._closedPromise_resolve = null;
this._closedPromise_reject = null;
}
};
}

class ReadableStreamReader {
constructor(stream) {
Expand Down Expand Up @@ -339,7 +306,6 @@ function CancelReadableStream(stream, reason) {
function CloseReadableStream(stream) {
assert(stream._state === 'readable');

stream._resolveClosedPromise(undefined);
stream._state = 'closed';

if (IsReadableStreamLocked(stream) === true) {
Expand Down Expand Up @@ -425,7 +391,6 @@ function CreateReadableStreamErrorFunction(stream) {
}

stream._queue = [];
stream._rejectClosedPromise(e);
stream._storedError = e;
stream._state = 'errored';

Expand Down
30 changes: 16 additions & 14 deletions reference-implementation/test/bad-underlying-sources.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ test('Underlying source: throwing pull getter (initial pull)', t => {
}
});

rs.closed.then(
rs.getReader().closed.then(
() => t.fail('closed should not fulfill'),
r => t.equal(r, theError, 'closed should reject with the thrown error')
);
Expand All @@ -52,7 +52,7 @@ test('Underlying source: throwing pull method (initial pull)', t => {
}
});

rs.closed.then(
rs.getReader().closed.then(
() => t.fail('closed should not fulfill'),
r => t.equal(r, theError, 'closed should reject with the thrown error')
);
Expand All @@ -73,10 +73,11 @@ test('Underlying source: throwing pull getter (second pull)', t => {
throw theError;
}
});
const reader = rs.getReader();

rs.getReader().read().then(r => t.deepEqual(r, { value: 'a', done: false }, 'the chunk read should be correct'));
reader.read().then(r => t.deepEqual(r, { value: 'a', done: false }, 'the chunk read should be correct'));

rs.closed.then(
reader.closed.then(
() => t.fail('closed should not fulfill'),
r => t.equal(r, theError, 'closed should reject with the thrown error')
);
Expand All @@ -97,10 +98,11 @@ test('Underlying source: throwing pull method (second pull)', t => {
}
}
});
const reader = rs.getReader();

rs.getReader().read().then(r => t.deepEqual(r, { value: 'a', done: false }, 'the chunk read should be correct'));
reader.read().then(r => t.deepEqual(r, { value: 'a', done: false }, 'the chunk read should be correct'));

rs.closed.then(
reader.closed.then(
() => t.fail('closed should not fulfill'),
r => t.equal(r, theError, 'closed should reject with the thrown error')
);
Expand Down Expand Up @@ -152,7 +154,7 @@ test('Underlying source: throwing strategy getter', t => {
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: throwing strategy.size getter', t => {
Expand All @@ -173,7 +175,7 @@ test('Underlying source: throwing strategy.size getter', t => {
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: throwing strategy.size method', t => {
Expand All @@ -194,7 +196,7 @@ test('Underlying source: throwing strategy.size method', t => {
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: throwing strategy.shouldApplyBackpressure getter', t => {
Expand All @@ -215,7 +217,7 @@ test('Underlying source: throwing strategy.shouldApplyBackpressure getter', t =>
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: throwing strategy.shouldApplyBackpressure method', t => {
Expand All @@ -236,7 +238,7 @@ test('Underlying source: throwing strategy.shouldApplyBackpressure method', t =>
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: strategy.size returning NaN', t => {
Expand All @@ -263,7 +265,7 @@ test('Underlying source: strategy.size returning NaN', t => {
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: strategy.size returning -Infinity', t => {
Expand All @@ -290,7 +292,7 @@ test('Underlying source: strategy.size returning -Infinity', t => {
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});

test('Underlying source: strategy.size returning +Infinity', t => {
Expand All @@ -317,5 +319,5 @@ test('Underlying source: strategy.size returning +Infinity', t => {
}
});

rs.closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
rs.getReader().closed.catch(e => t.equal(e, theError, 'closed should reject with the error'));
});
7 changes: 0 additions & 7 deletions reference-implementation/test/brand-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ test('Can get the ReadableStreamReader constructor indirectly', t => {

function fakeReadableStream() {
return {
get closed() { return Promise.resolve(); },
cancel(reason) { return Promise.resolve(); },
pipeThrough({ writable, readable }, options) { return readable; },
pipeTo(dest, { preventClose, preventAbort, preventCancel } = {}) { return Promise.resolve(); },
Expand Down Expand Up @@ -109,12 +108,6 @@ function methodThrows(t, obj, methodName, target) {
t.throws(() => method.call(target), /TypeError/, methodName + ' should throw a TypeError');
}

test('ReadableStream.prototype.closed enforces a brand check', t => {
t.plan(2);
getterRejects(t, ReadableStream.prototype, 'closed', fakeReadableStream());
getterRejects(t, ReadableStream.prototype, 'closed', realWritableStream());
});

test('ReadableStream.prototype.cancel enforces a brand check', t => {
t.plan(2);
methodRejects(t, ReadableStream.prototype, 'cancel', fakeReadableStream());
Expand Down
27 changes: 11 additions & 16 deletions reference-implementation/test/pipe-to.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,17 @@ test('Piping from a ReadableStream from which lots of data are readable synchron

t.equal(ws.state, 'writable', 'writable stream state should start out writable');

let rsClosed = false;
rs.closed.then(() => {
rsClosed = true;
});

let pipeFinished = false;
rs.pipeTo(ws).then(
() => {
pipeFinished = true;
t.equal(rsClosed, true, 'readable stream should be closed after pipe finishes');
rs.getReader().closed.then(() => {
t.pass('readable stream should be closed after pipe finishes');
});
t.equal(ws.state, 'closed', 'writable stream state should be closed after pipe finishes');
},
e => t.error(e)
);
}
)
.catch(e => t.error(e));

setTimeout(() => {
t.equal(pipeFinished, true, 'pipe should have finished before a setTimeout(,0) since it should only be microtasks');
Expand Down Expand Up @@ -70,19 +67,17 @@ test('Piping from a ReadableStream in readable state to a WritableStream in clos
ws.close();
t.equal(ws.state, 'closing', 'writable stream should be closing immediately after closing it');

let rsClosed = false;
rs.closed.then(() => {
rsClosed = true;
});

rs.pipeTo(ws).then(
() => t.fail('promise returned by pipeTo should not fulfill'),
r => {
t.equal(r, cancelReason,
'the pipeTo promise should reject with the same error as the underlying source cancel was called with');
t.equal(rsClosed, true, 'readable stream should be closed after pipe finishes');
rs.getReader().closed.then(() => {
t.pass('readable stream should be closed after pipe finishes');
});
}
);
)
.catch(e => t.error(e));
});

test('Piping from a ReadableStream in readable state to a WritableStream in errored state', t => {
Expand Down
4 changes: 2 additions & 2 deletions reference-implementation/test/readable-stream-cancel.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ test('ReadableStream cancellation: cancel() on a locked stream should fail and n
result => t.deepEqual(result, { value: 'a', done: false }, 'read() should still work after the attempted cancel')
);

rs.closed.then(() => t.pass('closed should fulfill without underlying source cancel ever being called'));
reader.closed.then(() => t.pass('closed should fulfill without underlying source cancel ever being called'));
});

test('ReadableStream cancellation: returning a value from the underlying source\'s cancel should not affect the ' +
Expand Down Expand Up @@ -186,7 +186,7 @@ test('ReadableStream cancellation: cancelling before start finishes should preve
}
});

Promise.all([rs.cancel(), rs.closed]).then(() => {
Promise.all([rs.cancel(), rs.getReader().closed]).then(() => {
t.pass('pull should never have been called');
t.end();
})
Expand Down
Loading

0 comments on commit 1bfbd5e

Please sign in to comment.