Adapt streams for additional stackTrace argument.

Adapt streams for additional stackTrace argument.

R=lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28511

[floitsch, 2013-10-03 12:54:18]

In several cases I provide a catchError((e, [st]) => ...) instead of
catchError((e, st) => ...)
(same for handleError)
In theory this should allow the functions to be used with older code, but if
they are only attached to our internal futures/streams then this is unnecessary.
If you spot bad usages, please mark them, and I will make the optional argument
required.

[Lasse Reichstein Nielsen, 2013-10-04 08:45:17]

lgtm

https://codereview.chromium.org/25094002/diff/6001/pkg/barback/lib/src/utils....
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/barback/lib/src/utils....
pkg/barback/lib/src/utils.dart:142: },
just "controller.add", no need to eta-expand.

https://codereview.chromium.org/25094002/diff/6001/pkg/barback/lib/src/utils....
pkg/barback/lib/src/utils.dart:143: onError: controller.addError,
Is indentation right here. I'd like it to be indented by at least two.
Maybe:
 stream.listen(
   controller.add,
   onError: controller.addError,
   onDone: () {
     ...  
   }
 );

https://codereview.chromium.org/25094002/diff/6001/pkg/http/test/safe_http_se...
File pkg/http/test/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/http/test/safe_http_se...
pkg/http/test/safe_http_server.dart:49: }, onError: (error, [StackTrace
stackTrace]) {
Make not optional. This is our library class that we are calling listen on, it
should match.

https://codereview.chromium.org/25094002/diff/6001/pkg/http/test/safe_http_se...
pkg/http/test/safe_http_server.dart:60: if (onError is ZoneBinaryCallback) {
Consider making a helper function: takesTwoArguments(onError)
Much more readable, and if we get a better way to do it, we only need to change
in one place.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/schedule.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/schedule.dart:448: }).catchError((e, [StackTrace
trace]) {
This is Future, not Stream?

Make not optional. We assume that whenComplete is the method we inherit from
Future.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/schedule.dart:457: }).catchError((e, [StackTrace
stackTrace]) {
Make not optional.
Assume .then is our own.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/schedule.dart:462: return
_noPendingCallbacks.catchError((e, [StackTrace stackTrace]) {
not optional.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/scheduled_server/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/scheduled_server/safe_http_server.dart:49: },
onError: (error, [StackTrace stackTrace]) {
make not optional.

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
File pkg/sequence_zip/lib/stream_zip.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
pkg/sequence_zip/lib/stream_zip.dart:52: void handleError(Object error,
[StackTrace stackTrace]) {
Make not optional.

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
pkg/sequence_zip/lib/stream_zip.dart:60: void handleErrorCancel(Object error,
[StackTrace stackTrace]) {
Make not optional.

https://codereview.chromium.org/25094002/diff/6001/pkg/watcher/lib/src/direct...
File pkg/watcher/lib/src/directory_watcher.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/watcher/lib/src/direct...
pkg/watcher/lib/src/directory_watcher.dart:136: }, onError: (error, [StackTrace
stackTrace]) {
Not optional. Stream is from io-lib.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/io.dart:466: // TODO(floitsch): Sink.addError
without stack trace.
What is the solution?
Should EventSink.addError have signature addError(error, [StackTrace trace)?

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/safe_http_server.dart:71: if (onError is
ZoneBinaryCallback) {
Hmm, perhaps event make a helperFunction:
  callOneOrTwo(onError, error, stackTrace) 
that calls with one or two arguments as supported by onError.
I.e., abstract this entire if-statement into a helper function.
Name may vary.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:328: // TODO(nweiz): remove this when
issue 7964 is fixed.
7964 won't be fixed. This method won't work either. Is it used?

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:357: // TODO(nweiz): remove this when
issue 7787 is fixed.
It is fixed. If the solution doesn't solve this problem well enough (which is
likely) the comment can still be removed.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:238: * received an error without stack trace.
"without a stack trace".

What happens if onError is of none of the two types? E.g., void onError(int x,
int y) ?

How about:
"If [onError] has the latter type, accepting a second `stackTrace` argument, it
is called with two arguments, otherwise it is called with just the error
object."

[floitsch, 2013-10-05 18:11:47]

Adding Nathan and Bob for their packages and libraries.

https://codereview.chromium.org/25094002/diff/6001/pkg/barback/lib/src/utils....
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/barback/lib/src/utils....
pkg/barback/lib/src/utils.dart:142: },
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> just "controller.add", no need to eta-expand.

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/barback/lib/src/utils....
pkg/barback/lib/src/utils.dart:143: onError: controller.addError,
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Is indentation right here. I'd like it to be indented by at least two.
> Maybe:
>  stream.listen(
>    controller.add,
>    onError: controller.addError,
>    onDone: () {
>      ...  
>    }
>  );

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/http/test/safe_http_se...
File pkg/http/test/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/http/test/safe_http_se...
pkg/http/test/safe_http_server.dart:49: }, onError: (error, [StackTrace
stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Make not optional. This is our library class that we are calling listen on, it
> should match.

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/http/test/safe_http_se...
pkg/http/test/safe_http_server.dart:60: if (onError is ZoneBinaryCallback) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Consider making a helper function: takesTwoArguments(onError)
> Much more readable, and if we get a better way to do it, we only need to
change
> in one place.

Will do it in the async library, but not outside it.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/schedule.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/schedule.dart:448: }).catchError((e, [StackTrace
trace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> This is Future, not Stream?
> 
> Make not optional. We assume that whenComplete is the method we inherit from
> Future.

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/schedule.dart:457: }).catchError((e, [StackTrace
stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Make not optional.
> Assume .then is our own.

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/schedule.dart:462: return
_noPendingCallbacks.catchError((e, [StackTrace stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> not optional.

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/scheduled_server/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/scheduled_server/safe_http_server.dart:49: },
onError: (error, [StackTrace stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> make not optional.

Done.

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
File pkg/sequence_zip/lib/stream_zip.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
pkg/sequence_zip/lib/stream_zip.dart:52: void handleError(Object error,
[StackTrace stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Make not optional.

Done.
Are you sure? The closure will be given to the streams in line 75. If these are
"old" streams it will break them.

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
pkg/sequence_zip/lib/stream_zip.dart:60: void handleErrorCancel(Object error,
[StackTrace stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Make not optional.

Done.
ditto.

https://codereview.chromium.org/25094002/diff/6001/pkg/watcher/lib/src/direct...
File pkg/watcher/lib/src/directory_watcher.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/watcher/lib/src/direct...
pkg/watcher/lib/src/directory_watcher.dart:136: }, onError: (error, [StackTrace
stackTrace]) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Not optional. Stream is from io-lib.

Done.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/io.dart:466: // TODO(floitsch): Sink.addError
without stack trace.
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> What is the solution?
> Should EventSink.addError have signature addError(error, [StackTrace trace)?

Yes. See follow-up CLs.
This is one of the reasons this change is breaking.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/safe_http_server.dart:71: if (onError is
ZoneBinaryCallback) {
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> Hmm, perhaps event make a helperFunction:
>   callOneOrTwo(onError, error, stackTrace) 
> that calls with one or two arguments as supported by onError.
> I.e., abstract this entire if-statement into a helper function.
> Name may vary.

Will do inside dart:async.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:328: // TODO(nweiz): remove this when
issue 7964 is fixed.
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> 7964 won't be fixed. This method won't work either. Is it used?

Seems to be used in one place in pub. Can't tell if stream.first wouldn't work
there.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:357: // TODO(nweiz): remove this when
issue 7787 is fixed.
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> It is fixed. If the solution doesn't solve this problem well enough (which is
> likely) the comment can still be removed.

I prefer not to deal with completely independent TODOs in this CL.
There is already enough going on. No need to add noise.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:238: * received an error without stack trace.
On 2013/10/04 08:45:17, Lasse Reichstein Nielsen wrote:
> "without a stack trace".
> 
> What happens if onError is of none of the two types? E.g., void onError(int x,
> int y) ?
That's an error ;)

> 
> How about:
> "If [onError] has the latter type, accepting a second `stackTrace` argument,
it
> is called with two arguments, otherwise it is called with just the error
> object."
> 

   * The [onError] callback must be of type `void onError(error)` or
   * `void onError(error, StackTrace stackTrace)`. If [onError] accepts
   * two arguments it is called with the stack trace (which could be `null` if
   * the stream itself received an error without stack trace).
   * Otherwise it is called with just the error object.
done.

[Lasse Reichstein Nielsen, 2013-10-07 11:55:48]

Still LGTM

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
File pkg/sequence_zip/lib/stream_zip.dart (right):

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
pkg/sequence_zip/lib/stream_zip.dart:52: void handleError(Object error,
[StackTrace stackTrace]) {
Ack. Not optional yet then.

https://codereview.chromium.org/25094002/diff/6001/pkg/sequence_zip/lib/strea...
pkg/sequence_zip/lib/stream_zip.dart:60: void handleErrorCancel(Object error,
[StackTrace stackTrace]) {
Not here either.

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/25094002/diff/6001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:238: * received an error without stack trace.
Ah, it's an error, but we don't say which error it is.
Perfectly fine.

From "if .. accepts two arguments", I'd assume that it gets called with two
arguments, even if the types don't match, and in unchecked mode, it'll work.
Which is better than what I asked for - no need to make it complicated for
ourselves.

If onError requires three arguments, does it then "accept two arguments"? (I'd
argue "no", and again it's not something we have to write out, it's just an
error).

[nweiz, 2013-10-07 18:49:51]

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/packa...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:86: }, onError: (error, [StackTrace
stackTrace]) {
Does this mean that [getAttachedStackTrace] is no longer the primary means of
tracking tracking stack traces with errors in streams?

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/strea...
File pkg/barback/lib/src/stream_replayer.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/strea...
pkg/barback/lib/src/stream_replayer.dart:37: }, onError: (error, [StackTrace
stackTrace]) {
[_buffer] now needs to track the stack trace along with errors. It should
probably be an Either<T, Pair<dynamic, StackTrace>> now.

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/utils...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/utils...
pkg/barback/lib/src/utils.dart:146: });
The indentation here is wrong. It should be:

stream.listen(
    controller.add,
    onError: controller.addError,
    onDone: () {
  doneCount++;
  if (odneCount == streams.length) controller.close();
});

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/utils...
pkg/barback/lib/src/utils.dart:192: }).catchError((e, StackTrace stackTrace) {
Should this stackTrace parameter be optional? It's optional for [stream.listen].

https://codereview.chromium.org/25094002/diff/18001/pkg/http/test/safe_http_s...
File pkg/http/test/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/http/test/safe_http_s...
pkg/http/test/safe_http_server.dart:49: }, onError: (error, StackTrace
stackTrace) {
Why is this parameter mandatory when other stream stackTrace parameters are
optional?

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/safe_http_server.dart:55: { Function onError, void
onDone(),
No space before "Function"

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/async/broadcast_s...
File sdk/lib/async/broadcast_stream_controller.dart (right):

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/async/broadcast_s...
sdk/lib/async/broadcast_stream_controller.dart:33: Function onError,
Are you planning on adding documentation that explains the valid formats for
[onError] later? If you're removing the descriptive type annotation, there's no
way for a new user to know.

[nweiz, 2013-10-07 20:34:34]

One more thing I forgot to mention: this has a lot of type annotations for
closure parameters, which isn't the style we use in pub/pkg.

[floitsch, 2013-10-10 14:22:51]

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/packa...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/packa...
pkg/barback/lib/src/package_graph.dart:86: }, onError: (error, [StackTrace
stackTrace]) {
On 2013/10/07 18:49:51, nweiz wrote:
> Does this mean that [getAttachedStackTrace] is no longer the primary means of
> tracking tracking stack traces with errors in streams?

Yes. 'getAttachedStackTrace' will be deprecated.

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/strea...
File pkg/barback/lib/src/stream_replayer.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/strea...
pkg/barback/lib/src/stream_replayer.dart:37: }, onError: (error, [StackTrace
stackTrace]) {
On 2013/10/07 18:49:51, nweiz wrote:
> [_buffer] now needs to track the stack trace along with errors. It should
> probably be an Either<T, Pair<dynamic, StackTrace>> now.

Done.

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/utils...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/utils...
pkg/barback/lib/src/utils.dart:146: });
On 2013/10/07 18:49:51, nweiz wrote:
> The indentation here is wrong. It should be:
> 
> stream.listen(
>     controller.add,
>     onError: controller.addError,
>     onDone: () {
>   doneCount++;
>   if (odneCount == streams.length) controller.close();
> });

Done.

https://codereview.chromium.org/25094002/diff/18001/pkg/barback/lib/src/utils...
pkg/barback/lib/src/utils.dart:192: }).catchError((e, StackTrace stackTrace) {
On 2013/10/07 18:49:51, nweiz wrote:
> Should this stackTrace parameter be optional? It's optional for
[stream.listen].

We know where the catchError comes from. (It's from one of our Controllers).
Therefore we know it knows how to deal with 2 arg functions.
We use the optional 2nd argument when the receive might be a user-supplied
Future/Stream that doesn't know anything about 2-arg handlers yet. (To minimize
breakage).

https://codereview.chromium.org/25094002/diff/18001/pkg/http/test/safe_http_s...
File pkg/http/test/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/18001/pkg/http/test/safe_http_s...
pkg/http/test/safe_http_server.dart:49: }, onError: (error, StackTrace
stackTrace) {
On 2013/10/07 18:49:51, nweiz wrote:
> Why is this parameter mandatory when other stream stackTrace parameters are
> optional?

Same as before: we know that the receive knows how to deal with stacktraces.

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/safe_http_server.dart (right):

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/safe_http_server.dart:55: { Function onError, void
onDone(),
On 2013/10/07 18:49:51, nweiz wrote:
> No space before "Function"

Done.

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/async/broadcast_s...
File sdk/lib/async/broadcast_stream_controller.dart (right):

https://codereview.chromium.org/25094002/diff/18001/sdk/lib/async/broadcast_s...
sdk/lib/async/broadcast_stream_controller.dart:33: Function onError,
On 2013/10/07 18:49:51, nweiz wrote:
> Are you planning on adding documentation that explains the valid formats for
> [onError] later? If you're removing the descriptive type annotation, there's
no
> way for a new user to know.

(Public facing) documentation has already been updated. If we forgot functions,
please let us know.

[floitsch, 2013-10-10 14:56:53]

Removed types in closures for Seattle-owned packages and libraries.

[floitsch, 2013-10-11 12:02:45]

Committed patchset #6 manually as r28511 (presubmit successful).