Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bigtable this.end() does not stop the stream? #2271

Closed
arbesfeld opened this issue May 2, 2017 · 8 comments
Closed

Bigtable this.end() does not stop the stream? #2271

arbesfeld opened this issue May 2, 2017 · 8 comments
Assignees
Labels
api: bigtable Issues related to the Bigtable API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects

Comments

@arbesfeld
Copy link
Contributor

Using Bigtable 0.9.1 - I seem to be getting more than 1 result after calling this.end(). Is this expected behavior?

@stephenplusplus stephenplusplus self-assigned this May 2, 2017
@stephenplusplus
Copy link
Contributor

I recall seeing this discussed in another issue, and it might be an issue with one of our dependencies. I will look into it. Thanks for reporting.

@stephenplusplus
Copy link
Contributor

The issue I was thinking of doesn't actually relate to the Bigtable API, so throw that theory out the window.

I tried to reproduce, but couldn't. My script and results:

var bigtable = require('@google-cloud/bigtable')()

bigtable.getInstancesStream()
  .on('data', function (instance) {
    console.log('instance')
    this.end()
  })
  .on('end', function () {
    console.log('over')
  })

// output:
instance
over

Without the this.end():

var bigtable = require('@google-cloud/bigtable')()

bigtable.getInstancesStream()
  .on('data', function (instance) {
    console.log('instance')
  })
  .on('end', function () {
    console.log('over')
  })

// output:
instance
instance
instance
over

Are you using a different method? Could there be other code interfering with when this.end() is called? Seeing your code might help.

@stephenplusplus stephenplusplus added the api: bigtable Issues related to the Bigtable API. label May 2, 2017
@arbesfeld
Copy link
Contributor Author

We're using table.createReadStream(). Here's what it looks like:

    await new Promise((resolve, reject) => {
      table
        .createReadStream({
          decode: true,
          start: 'foo',
          end: 'bar'
          filter: [{
            column: {
              cellLimit: 1,
            },
          }],
        })
        .on('data', function onData(row) {
          this.end();
          resolve();
        })
        .on('end', resolve)
        .on('error', reject);
    });

Other methods could be using table at the same time, if that affects something?

@stephenplusplus
Copy link
Contributor

Would it be a problem that resolve() is called from the data handler and end? this.end() will gracefully exit the stream, which means that the end event will be called.

@arbesfeld
Copy link
Contributor Author

That would be acceptable for our application. We found that onData would continued to be called even after this.end(). We're on Node 7.9.0, and we found a workaround by introducing another variable outside of the promise.

@stephenplusplus stephenplusplus added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label May 3, 2017
@stephenplusplus
Copy link
Contributor

Found the issue. PR incoming.

@stephenplusplus
Copy link
Contributor

PR sent: #2276

@stephenplusplus stephenplusplus added this to Needs Review in State Jun 12, 2017
@stephenplusplus stephenplusplus moved this from Needs Review to In Progress in State Jun 12, 2017
@eoogbe eoogbe added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Aug 7, 2017
@lukesneeringer
Copy link
Contributor

This issue was moved to googleapis/nodejs-bigtable#6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
No open projects
State
In Progress
Development

No branches or pull requests

5 participants