Refactor Storage read and write streams

[stephenplusplus]

Related: #340, #362, #423, #435, #436 #809 (comment)

This PR will force a breaking change. But, a breaking change for the better.

We will no longer have to tell users to hook on to the "complete" events. That comes from the request module, and while that is quite a popular module, a user who creates a write or read stream from us doesn't really expect it, as they don't know we're using request underneath. It's like we're forcing them to learn our dependency, which is no good.

In the case of read streams, the user always received "end", but now we've removed the "complete" event. The use can safely listen for end and error, as that will give them all they need. "complete" used to emit with it the response from the API, but we update the file metadata with this object, so they can access the same from "file.metadata" inside their end callback*.

For write streams, we hold off letting the stream "finish" now until we know the data was uploaded successfully. This means we don't have to let it finish, then do post-processing, then do complete. That was messy.

Any questions, please ask! Streams, man.

* Mention in release notes.

Read Streams

Before

file.createReadStream()
  .on('data', function() {})
  .on('complete', function(metadata) {});

After

file.createReadStream()
  .on('data', function() {})
  .on('end', function() {
    // file.metadata === old "complete.metadata" argument
  });

Write Streams

Before

var writeStream = file.createWriteStream();
writeStream.on('complete', function(metadata) {});
writeStream.end('data');

After

var writeStream = file.createWriteStream();
writeStream.on('finish', function() {
  // file.metadata === old "complete.metadata" argument
});
writeStream.end('data');

[jgeewax]

Do you have an example of before / after code snippets? Just so we can clearly communicate to our users what needs to change?

[stephenplusplus]

Yep, updated.

[jgeewax]

Hmm some of this seems weird to me.

1. Couldn't we pass the metadata along with the event to keep the callbacks backwards compatible?
2. Read streams: couldn't we support listening on "end" as an alias for "complete"
3. Write streams: couldn't we support listening on "complete" as an alias for "finish" ?

From my (outsider's) perspective, it seems like a bunch of renames that we could have potentially hidden from users. I'm also scared that things don't look super consistent:

• Read: end -> complete
• Write: complete -> finish

These all sound like synonyms to me... Is it crazy to suggest maybe we call everything the same? (Say, all complete ?)

[stephenplusplus]

Well, the thing I actually did was "make our streams not do anything special". "end" and "finish" are actual, Node streams core events that all streams have. We were dancing around them, doing post-processing, forcing us to hide native events and rename them as "complete".

Couldn't we pass the metadata along with the event to keep the callbacks backwards compatible?

Because they are not custom events now (end for readable, and finish for writable), we can't do anything special, such as passing metadata.

Read streams: couldn't we support listening on "end" as an alias for "complete"
Write streams: couldn't we support listening on "complete" as an alias for "finish" ?

We shouldn't do anything custom because:

A) a user will have to learn what we created, instead of using what they expect from all other streams they use in Node
B) compatibility in a pipeline will be broken. If we are stream A and they pipe to stream B, B expects A to follow stream conventions.

I understand backwards compatibility would be nice, but I would urge us not to do this for the same reason we don't fix bugs and carry around "shim bugs". Our code gets weird and our users don't know what the heck's going on. We just need to make a note of it in the release notes and play by semver. A breaking API is expected in 0.x bumps.

[stephenplusplus]

These all sound like synonyms to me... Is it crazy to suggest maybe we call everything the same? (Say, all complete ?)

Nope, and I wish they were. Streams are super weird, and they're working to improve on them. I really hope it gets more intuitive. (See https://github.com/stephenplusplus/stream-faqs for tracking how difficult they can be 😕)

[callmehiphop]

huge 👍

a user will have to learn what we created, instead of using what they expect from all other streams they use in Node

This sums up why I love this pull request.

[stephenplusplus]

Updated the first post with many references of the confusion our event juggling has caused.

[stephenplusplus]

PTAL

[callmehiphop]

Looks good to me! Rebase time?

[stephenplusplus]

Does it need to be?

[callmehiphop]

It doesn't, this can bask in PR limbo for the rest of time!

[stephenplusplus]

Oh, like squashing. Sorry, thought you meant it wouldn't merge without a rebase.

I can rebase, but the commits here are fine separated. It's easier to tear apart later / attribute regressions to if we ever needed to.

[callmehiphop]

Fine by me!

[jseeley78]

Is anyone else having problems with this change besides me? All my writestreams (uploads) now come back with "The uploaded data did not match the data from the server. As a precaution, the file has been deleted. To be sure the content is the same, you should try uploading the file again."

[stephenplusplus]

We haven't heard of anything, and our tests seem to work properly. Are there any commonalities between the types of transfers you're doing that fail?

[jseeley78]

all my cases are piping from imagemagick;

var wr = gfs.createWriteStream(dst, {bucket: gfs.Buckets.archiveBucket});
gm(src)
    .setFormat('png')
    .stream(function(err, stdout, stderr) {
    if (err) return cb(err);
    // pipe output to writestream
    stdout.pipe(wr);
    // on writestream complete; make public/return
     wr.on("complete", function() {
        gfs.makePublic(dst, {bucket: gfs.Buckets.archiveBucket}, cb);
    });
});

my createWriteStream function looks like this->

GFS.prototype.createWriteStream = function(filename, options) {
    options = options || {};
    return this.getBucket(options.bucket).file(filename).createWriteStream({
        metadata: {contentType: this.getContentType(filename)}
    });
};

[stephenplusplus]

Thanks! What happens if you listen for finish instead of complete? Also, listen for error on wr as well.

[jseeley78]

I don't get the new finished event; I tried listening for finish, complete, end, data and error; I'm only getting back on "error"->

{ [Error: The uploaded data did not match the data from the server. As a precaution, the file has been deleted. To be sure the content is the same, you should try uploading the file again.] code: 'FILE_NO_UPLOAD', errors: [ null ] }

for what its worth is does work just fine when I'm using this via a standard fs.read->

var rd = fs.createReadStream(source),
      file = this.getBucket(bucket).file(target);
      wr = file.createWriteStream({ metadata: { contentType: contentType } });
        wr.on("finish", function() {
        file.acl.add({
            entity: 'allUsers',
            role: gcloud.storage.acl.READER_ROLE
        }, cb);
    });
    rd.pipe(wr);

so it seems to be something with the way node graphicsmagic(imagemagic) is handling the stream. It works fine with release 20 and earlier though...

[stephenplusplus]

Could you open a new issue so we can keep digging into this? Also please include a link to the library and I'll try to reproduce.

[jseeley78]

ok, submitted as ticket #889