Rely on stream.destroy() whenever available

[kanongil]

Also updates a bunch of stream closing related test.

ReadableStream has included a .destroy() method since node 8. People can still provide older streams through old readable-stream module implementations.

Btw. non-destroyable streams should probably just error when returned from a handler, but that is a breaking change.

[kanongil]

Anyone want to have a look at this?

[devinivy]

This looks good. It's great to have these additional tests and to stay up to date with node. I don't detect any breaking changes since destroy() effectively constitutes a close() (e.g. including events that are subsequently emitted), and as far as I can tell should always take care of unpiping.

[kanongil]

This looks good. It's great to have these additional tests and to stay up to date with node. I don't detect any breaking changes since destroy() effectively constitutes a close() (e.g. including events that are subsequently emitted), and as far as I can tell should always take care of unpiping.

Yeah, the stream integration is quite defensive, as streams were not that aligned during the earlier implementation. The story is much better now, and we can mostly just trust that client streams are somewhat well-behaved.

I'll see about re-basing this PR, and create a followup with a breaking change to deprecate any streams that don't include a destroy() method.

[kanongil]

Incidentally, shot is one such broken client, as per #4149 & hapijs/shot#129 😬

[lloydbenson]

As usual thanks for looking into this @kanongil. I am sure you probably noticed already but there is a conflict.

[kanongil]

Rebased, and aliased the readable-stream require to clearly label it as a legacy module.

[Nargonath]

I don't have an extensive knowledge about Streams but this LGTM. I also like the fact that we are following Node improvements.

There are also still some merge conflicts.