Skip to content

Storage streams use 'complete' event for end of stream but built-in streams use 'finish'  #362

Description

@ryanseys

As was raised in #340, I looked into why there was a discrepancy between what the developer thought and what was the real case. There was a suggestion made to update our docs, but our docs aren't the issue here. A snippet from the tests shows the issue:

Using 'finish' event:

file.createReadStream()
.pipe(fs.createWriteStream(tmpFilePath))
.on('error', done)
.on('finish', function() {
  file.delete(function(err) {
    assert.ifError(err);

    fs.readFile(tmpFilePath, function(err, data) {
      assert.equal(data, fileContent);
      done();
    });
  });
});

Using 'complete' event:

var file = bucket.file(filenames[0]);
fs.createReadStream(files.logo.path)
  .pipe(file.createWriteStream())
  .on('error', done)
  .on('complete', function() {
    file.copy(filenames[1], function(err, copiedFile) {
      assert.ifError(err);
      copiedFile.copy(filenames[2], done);
    });
  });

Seems the only difference is the type of file that is getting piped to. In the first case, it's a regular stream from fs and in the second it's our implementation of the storage file write stream.

So my question is, should we use a consistent finish event everywhere or is this by-design or otherwise okay?

Activity

  1. stephenplusplus commented on Jan 23, 2015

    @stephenplusplus
    Contributor

    Our complete event comes from request: https://github.com/request/request/blob/e33a883dd412dc7a1fdd1e1f282e840faa540609/request.js#L1224

    Being consistent with fs would be nice, but off hand, I'm not sure we can do that.

  2. ryanseys commented on Jan 30, 2015

    @ryanseys
    ContributorAuthor

    Are you sure? I thought we intercept pretty much every event and emit our own events.

    I'm looking at code here and here for example.

  3. stephenplusplus commented on Feb 2, 2015

    @stephenplusplus
    Contributor

    Ooh, I forgot about our new proxy stream. Yeah, we should be able to emit whatever we choose in these cases.

  4. modified the milestone: Storage Stable on Feb 2, 2015
  5. stephenplusplus commented on Feb 5, 2015

    @stephenplusplus
    Contributor

    This is quite complex.

    A native readable stream is done when it emits 'end', which is the same time the stream is .end()-ed. The end listener doesn't receive arguments, and if you pass arguments to stream.end(), they get written to the stream immediately prior to being ended. I wonder if this is why request uses complete, so they can do some post-processing before emitting the response headers/body/status arguments.

    I'm okay with keeping complete as the consistent event for readable and writable end signals.

  6. ryanseys commented on Feb 5, 2015

    @ryanseys
    ContributorAuthor

    Oh yeah, good observation. That looks like exactly what they are doing. I'm happy with this too. It just makes it look weird when you have pipe with different streams, but in either case you have to make sure you're using the right event. complete as a consistent "yo, im done now" event is good for me.

  7. 15 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

api: storageIssues related to the Cloud Storage API.type: questionRequest for information or clarification. Not an issue.

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions