Fix multipart transformer in package:mime

Fix multipart transformer in package:mime

The error codition occures in the following scenario:
A multipart transformer returns a new multipart, that part is listened on, but
the subscription for more parts is cancelled. This used to cause cancellation of
the byte stream into the multipart parser, which caused the first part to never
be read completely.

R=lrn@google.com, sgjesse@google.com

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

[kustermann, 2015-01-26 14:31:05]

kustermann@google.com changed reviewers:
+ lrn@google.com, sgjesse@google.com

[Søren Gjesse, 2015-01-26 16:07:54]

lgtm

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
File dart/pkg/mime/lib/src/bound_multipart_stream.dart (right):

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:124: void
_delayOperationUntilMultipartHasBeenRead(operation()) {
Nice!

[Lasse Reichstein Nielsen, 2015-01-27 15:06:40]

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
File dart/pkg/mime/lib/src/bound_multipart_stream.dart (right):

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:126:
_multipartController.done.then((_) => operation());
It's clever, but maybe a little too clever.
It depends on the then-listeners of the done-future being executed in the order
they are added. We do that, but I'm not sure if we ever promised to do it, and
can't promise that it can't break somehow, so I'd see it as slightly fragile
(and if you ever swap a pause and a resume, you're doomed!)

Further, it assumes that all these operations are executed before the next part
starts parsing, which may be true, but it's not obvious from reading this code
alone, and at the very least requires documentation.

All you need is the desired state of the part stream (active, paused n times, or
cancelled, one integer worth of information), and then act on that just before
starting the next part - that is, at the point where _multipartController is set
to null.

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:133:
_delayOperationUntilMultipartHasBeenRead(_subscription.resume);
so something like:
  if (_isPartActive && _inputState >= _PAUSE) {
    _inputState -= _PAUSE;
  } else {
    _subscription.resume();
  }

where 
  int _inputState = 0;
  const int _PAUSE = 2;
  const int _CANCEL = 1;
then set _inputState |= _CANCEL on cancel and _inputState += _PAUSE on pause,
and as above on resume.

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:297: },
Just
  onPause: _subscription.pause,
No need for the extra function wrapper.

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:319: _multipartController =
null;
So this is where you could check the flags set while multipartController was
non-null, and either pause or cancel as necessary. 

if (_inputState & _CANCEL != 0) { 
  _subscription.cancel();
} else {
  while (_inputState >= _PAUSE) {
    _subscription.pause();
    _inputState -= _PAUSE;
  }
}

[kustermann, 2015-02-12 10:01:58]

PTAL

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
File dart/pkg/mime/lib/src/bound_multipart_stream.dart (right):

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:126:
_multipartController.done.then((_) => operation());
On 2015/01/27 15:06:39, Lasse Reichstein Nielsen wrote:
> It's clever, but maybe a little too clever.
> It depends on the then-listeners of the done-future being executed in the
order
> they are added. We do that, but I'm not sure if we ever promised to do it, and
> can't promise that it can't break somehow, so I'd see it as slightly fragile
> (and if you ever swap a pause and a resume, you're doomed!)
> 
> Further, it assumes that all these operations are executed before the next
part
> starts parsing, which may be true, but it's not obvious from reading this code
> alone, and at the very least requires documentation.
> 
> All you need is the desired state of the part stream (active, paused n times,
or
> cancelled, one integer worth of information), and then act on that just before
> starting the next part - that is, at the point where _multipartController is
set
> to null.

Excellent idea.

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:133:
_delayOperationUntilMultipartHasBeenRead(_subscription.resume);
On 2015/01/27 15:06:39, Lasse Reichstein Nielsen wrote:
> so something like:
>   if (_isPartActive && _inputState >= _PAUSE) {
>     _inputState -= _PAUSE;
>   } else {
>     _subscription.resume();
>   }
> 
> where 
>   int _inputState = 0;
>   const int _PAUSE = 2;
>   const int _CANCEL = 1;
> then set _inputState |= _CANCEL on cancel and _inputState += _PAUSE on pause,
> and as above on resume.

As discussed offline, we get the pause event only once from the stream
controller `onPause` callback.

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:297: },
On 2015/01/27 15:06:39, Lasse Reichstein Nielsen wrote:
> Just
>   onPause: _subscription.pause,
> No need for the extra function wrapper.

Done.

https://codereview.chromium.org/874223002/diff/1/dart/pkg/mime/lib/src/bound_...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:319: _multipartController =
null;
On 2015/01/27 15:06:39, Lasse Reichstein Nielsen wrote:
> So this is where you could check the flags set while multipartController was
> non-null, and either pause or cancel as necessary. 
> 
> if (_inputState & _CANCEL != 0) { 
>   _subscription.cancel();
> } else {
>   while (_inputState >= _PAUSE) {
>     _subscription.pause();
>     _inputState -= _PAUSE;
>   }
> }

Done.

[Lasse Reichstein Nielsen, 2015-02-12 13:40:04]

lgtm

https://codereview.chromium.org/874223002/diff/20001/dart/pkg/mime/lib/src/bo...
File dart/pkg/mime/lib/src/bound_multipart_stream.dart (right):

https://codereview.chromium.org/874223002/diff/20001/dart/pkg/mime/lib/src/bo...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:83: static const int
_CONTROLLER_STATE_CANCELLED = 3;
The US version is CANCELED with one L (as I have recently learned).

https://codereview.chromium.org/874223002/diff/20001/dart/pkg/mime/lib/src/bo...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:158: throw new Exception("This
code should never be reached.");
This should be an Error, not an Exception.
I suggest a StateError.

[kustermann, 2015-02-12 13:51:06]

New patchsets have been uploaded after l-g-t-m from lrn@google.com

[kustermann, 2015-02-12 13:51:25]

Thanks for the review :)

https://codereview.chromium.org/874223002/diff/20001/dart/pkg/mime/lib/src/bo...
File dart/pkg/mime/lib/src/bound_multipart_stream.dart (right):

https://codereview.chromium.org/874223002/diff/20001/dart/pkg/mime/lib/src/bo...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:83: static const int
_CONTROLLER_STATE_CANCELLED = 3;
On 2015/02/12 13:40:04, Lasse Reichstein Nielsen wrote:
> The US version is CANCELED with one L (as I have recently learned).

Good to know :)

Done.

https://codereview.chromium.org/874223002/diff/20001/dart/pkg/mime/lib/src/bo...
dart/pkg/mime/lib/src/bound_multipart_stream.dart:158: throw new Exception("This
code should never be reached.");
On 2015/02/12 13:40:04, Lasse Reichstein Nielsen wrote:
> This should be an Error, not an Exception.
> I suggest a StateError.

Done.

I was looking for something like `throw new UnreachableError()` but I could only
find `throw new UnimplementedError()`

[kustermann, 2015-02-12 13:51:27]

Committed patchset #3 (id:40001) manually as 43726 (presubmit successful).