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.

Sign up for GitHub

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

Jump to bottom

Bug fix - stop promisifying Job#promise and Operation#promise #1732

Merged
merged 3 commits into from
Oct 21, 2016
Merged

Bug fix - stop promisifying Job#promise and Operation#promise #1732

merged 3 commits into from
Oct 21, 2016

Conversation

jmdobry
Copy link
Contributor

@jmdobry jmdobry commented Oct 20, 2016 •
edited
Loading

This PR represents a test case allowing one to reproduce a bizarre bug I encountered when trying to use job.promise().

Checkout my branch and do the following:

  1. cd packages/bigquery
  2. Set the necessary env vars to run the system tests
  3. Do npm run system-test

The test will start, and the BigQuery import job will be created, and then the process will be killed inexplicably, with nothing else being printed to the console. I tested this in Node v4.2.0 and v6.7.0.

I believe that somehow, when the job attempts to resolve its promise after emitting the "complete" event, the process crashes. I dunno why.

EDIT: This was caused by the Job#promise and Operation#promise methods getting promisified.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2016
@stephenplusplus
Copy link
Contributor

If you add a catch block to both promises, I wonder if it would error out.

@jmdobry
Copy link
Contributor Author

jmdobry commented Oct 20, 2016

I think I might have figured it out. I think the Job.prototype.promise method is erroneously getting promisified. Investigating some more...

@jmdobry
Copy link
Contributor Author

jmdobry commented Oct 20, 2016

That was it! Some kind of crazy promise inception. I'll update my PR to fix it.

@jmdobry jmdobry changed the title Repro test for bizarre Bigquery job.promise bug. Bug fix - stop promisifying Job#promise and Operation#promise Oct 20, 2016
@callmehiphop
Copy link
Contributor

@jmdobry good catch - an easier fix would probably just be to ignore methods called promise by default. This has the added benefit of just releasing a patch to common as opposed to several patch releases.

@jmdobry
Copy link
Contributor Author

jmdobry commented Oct 20, 2016

Done

@callmehiphop
Copy link
Contributor

callmehiphop commented Oct 21, 2016 •
edited
Loading

Would you mind adding a unit test for this? An additional assertion in the current test should be good enough.

Edit: added a commit.

stephenplusplus
stephenplusplus suggested changes Oct 21, 2016
View reviewed changes
packages/bigquery/system-test/bigquery.js
.then(function(results) {
var metadata = results[0];
assert(metadata.status);
assert.equal(metadata.status.state, 'DONE');

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

packages/bigquery/system-test/bigquery.js
})
.then(function(results) {
var metadata = results[0];
assert(metadata.status);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

packages/bigquery/system-test/bigquery.js
return table.import(file)
.then(function(results) {
return results[0].promise();
})

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jmdobry
Copy link
Contributor Author

jmdobry commented Oct 21, 2016

LGTM

@stephenplusplus stephenplusplus merged commit 8000535 into googleapis:master Oct 21, 2016
@stephenplusplus stephenplusplus added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. core labels Oct 21, 2016
@stephenplusplus
Copy link
Contributor

Thanks for catching this and the fix, @jmdobry! We're planning a release today.

@jmdobry jmdobry deleted the bigquery branch October 21, 2016 19:39
This was referenced Jan 2, 2024
Open
Open
@takeratta takeratta mentioned this pull request Mar 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@callmehiphop callmehiphop callmehiphop left review comments

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. core type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmdobry @stephenplusplus @callmehiphop @googlebot