Skip to content

Commit

Permalink
Remove fallback from underlying sink abort to close
Browse files Browse the repository at this point in the history
See discussion in #620 and in particular the conclusion at #620 (comment).
  • Loading branch information
domenic committed Jan 6, 2017
1 parent e4b97e6 commit b412a96
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 53 deletions.
52 changes: 23 additions & 29 deletions index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -2649,7 +2649,7 @@ WritableStream(<var>underlyingSink</var> = {}, { <var>size</var>, <var>highWater
and put it in an errored state. It should clean up any held resources, much like <code>close</code>, but perhaps
with some custom handling. Unlike <code>close</code>, <code>abort</code> will be called even if writes are queued
up; those <a>chunks</a> will be thrown away. If this process is asynchronous, it can return a promise to signal
success or failure. If no abort method is passed, by default the <code>close</code> method will be called instead.
success or failure.
</ul>

The <code>controller</code> object passed to <code>start</code>, <code>write</code> and <code>close</code> is an
Expand Down Expand Up @@ -3321,8 +3321,8 @@ nothrow>WritableStreamDefaultControllerAbort ( <var>controller</var>, <var>reaso

<emu-alg>
1. Set _controller_.[[queue]] to a new empty List.
1. Let _sinkAbortPromise_ be ! PromiseInvokeOrFallbackOrNoop(_controller_.[[underlyingSink]],
`"abort"`, « _reason_ », `"close"`, « _controller_ »).
1. Let _sinkAbortPromise_ be ! PromiseInvokeOrNoop(_controller_.[[underlyingSink]], `"abort"`, «
_reason_ »).
1. Return the result of transforming _sinkAbortPromise_ by a fulfillment handler that returns
*undefined*.
</emu-alg>
Expand Down Expand Up @@ -3722,30 +3722,6 @@ A few abstract operations are used in this specification for utility purposes. W
1. Return *true*.
</emu-alg>

<h4 id="promise-invoke-or-fallback-or-noop" aoid="PromiseInvokeOrFallbackOrNoop" nothrow>PromiseInvokeOrFallbackOrNoop (
<var>O</var>, <var>P1</var>, <var>args1</var>, <var>P2</var>, <var>args2</var> )</h4>

<div class="note">
PromiseInvokeOrFallbackOrNoop is a specialized version of <a>promise-calling</a> that works on methods, calls a
fallback method if the first method is not present, and returns a promise for <emu-val>undefined</emu-val> when
neither method is present.
</div>

<emu-alg>
1. Assert: _O_ is not *undefined*.
1. Assert: ! IsPropertyKey(_P1_) is *true*.
1. Assert: _args1_ is a List.
1. Assert: ! IsPropertyKey(_P2_) is *true*.
1. Assert: _args2_ is a List.
1. Let _method_ be GetV(_O_, _P1_).
1. If _method_ is an abrupt completion, return <a>a promise rejected with</a> _method_.[[Value]].
1. Let _method_ be _method_.[[Value]].
1. If _method_ is *undefined*, return ! PromiseInvokeOrNoop(_O_, _P2_, _args2_).
1. Let _returnValue_ be Call(_method_, _O_, _args1_).
1. If _returnValue_ is an abrupt completion, return <a>a promise rejected with</a> _returnValue_.[[Value]].
1. Otherwise, return <a>a promise resolved with</a> _returnValue_.[[Value]].
</emu-alg>

<h4 id="promise-invoke-or-noop" aoid="PromiseInvokeOrNoop" nothrow>PromiseInvokeOrNoop ( <var>O</var>, <var>P</var>,
<var>args</var> )</h4>

Expand Down Expand Up @@ -4085,7 +4061,14 @@ promises returned by its <a lt="writable stream writer">writer</a>'s {{WritableS
close() {
return new Promise((resolve, reject) => {
ws.onclose = resolve;
ws.close();
ws.close(1000);
});
},

abort(reason) {
return new Promise((resolve, reject) => {
ws.onclose = resolve;
ws.close(4000, reason && reason.message);
});
}
});
Expand Down Expand Up @@ -4129,6 +4112,10 @@ an individual write succeeded or failed.

close() {
return fs.close(fd);
},

abort() {
return fs.close(fd);
}
});
}
Expand Down Expand Up @@ -4223,7 +4210,14 @@ source abstractions.
this._ws.close();
});
}
});

abort(reason) {
return new Promise((resolve, reject) => {
ws.onclose = resolve;
ws.close(4000, reason && reason.message);
});
}
}
</code></pre>

We can then use the objects created by this function to communicate with a remote web socket, using the standard stream
Expand Down
10 changes: 0 additions & 10 deletions reference-implementation/lib/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,16 +100,6 @@ exports.PromiseInvokeOrPerformFallback = (O, P, args, F, argsF) => {
}
};

exports.PromiseInvokeOrFallbackOrNoop = (O, P1, args1, P2, args2) => {
assert(O !== undefined);
assert(IsPropertyKey(P1));
assert(Array.isArray(args1));
assert(IsPropertyKey(P2));
assert(Array.isArray(args2));

return exports.PromiseInvokeOrPerformFallback(O, P1, args1, exports.PromiseInvokeOrNoop, [O, P2, args2]);
};

// Not implemented correctly
exports.SameRealmTransfer = O => O;

Expand Down
7 changes: 3 additions & 4 deletions reference-implementation/lib/writable-stream.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';
const assert = require('assert');
const { InvokeOrNoop, PromiseInvokeOrNoop, PromiseInvokeOrFallbackOrNoop, ValidateAndNormalizeQueuingStrategy,
typeIsObject } = require('./helpers.js');
const { InvokeOrNoop, PromiseInvokeOrNoop, ValidateAndNormalizeQueuingStrategy, typeIsObject } =
require('./helpers.js');
const { rethrowAssertionErrorRejection } = require('./utils.js');
const { DequeueValue, EnqueueValueWithSize, GetTotalQueueSize, PeekQueueValue } = require('./queue-with-sizes.js');

Expand Down Expand Up @@ -574,8 +574,7 @@ class WritableStreamDefaultController {
function WritableStreamDefaultControllerAbort(controller, reason) {
controller._queue = [];

const sinkAbortPromise = PromiseInvokeOrFallbackOrNoop(controller._underlyingSink, 'abort', [reason],
'close', [controller]);
const sinkAbortPromise = PromiseInvokeOrNoop(controller._underlyingSink, 'abort', [reason]);
return sinkAbortPromise.then(() => undefined);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,15 +250,14 @@ promise_test(() => {
}, 'Aborting a WritableStream after it is closed is a no-op');

promise_test(t => {
// Testing that per https://github.com/whatwg/streams/issues/620#issuecomment-263483953 the fallback to close was
// removed.

// Cannot use recordingWritableStream since it always has an abort
let controller;
let closeArgs;
let closeCalled = false;
const ws = new WritableStream({
start(c) {
controller = c;
},
close(...args) {
closeArgs = args;
close() {
closeCalled = true;
}
});

Expand All @@ -267,9 +266,9 @@ promise_test(t => {
writer.abort();

return promise_rejects(t, new TypeError(), writer.closed, 'closed should reject with a TypeError').then(() => {
assert_array_equals(closeArgs, [controller], 'close must have been called, with the controller as its argument');
assert_false(closeCalled, 'close must not have been called');
});
}, 'WritableStream should call underlying sink\'s close if no abort is supplied');
}, 'WritableStream should NOT call underlying sink\'s close if no abort is supplied (historical)');

promise_test(() => {
let thenCalled = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,9 @@ promise_test(t => {
const writer2 = ws2.getWriter();
writer2.abort();

// Test PromiseInvokeOrFallbackOrNoop.
// Test abort() with a close underlying sink method present. (Historical; see
// https://github.com/whatwg/streams/issues/620#issuecomment-263483953 for what used to be
// tested here. But more coverage can't hurt.)
const ws3 = new WritableStream({
start: functionWithOverloads,
write: functionWithOverloads,
Expand Down

0 comments on commit b412a96

Please sign in to comment.