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

fix(core): fix another scenario that causes hanging (11.x) #513

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Mar 17, 2022

Related to #509, #490, and https://github.com/zapier/zapier/issues/53430, this addresses another scenario that causes the Lambda handler to hang/freeze.

Suppose a converted app has this (bad) legacy script:

var Zap = {
  bad_post_write: function(bundle, callback) {
    z.request({ ... }, function(err, response) {
      if (err) {
        callback(err);
      } else {
        callback(null, z.JSON.parse(response.content));
      }
    });
    callback(null, { message: 'ok' }); // double callback?!
  }
};

Notice how the post_write function returns immediately by calling callback(null, { message: 'ok' }) while makes an outbound z.request() and calls callback asynchronously.

When legacy-scripting-runner promisifies this post_write function, the callback in the z.request will come after the Lambda handler returns. Since we only end the logger once when the Lambda handler finishes, the HTTP logger corresponding to the z.request call, which comes after the Lambda handler, will stay open and never end until a long timeout.

To fix, we need to make sure to end any loggers allocated after the Lambda handler returns.

@eliangcs eliangcs marked this pull request as ready for review March 17, 2022 09:07
@eliangcs eliangcs requested a review from xavdid as a code owner March 17, 2022 09:07
@@ -55,8 +58,9 @@ const runLocally = (event) => {
handler(event, {}, (err, data) => {
if (err) {
reject(err);
} else {
resolve(data);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a problem while I was here. We should call either resolve or reject but not both.

Comment on lines +531 to +546
perform: (z, bundle) => {
https
.request(`${HTTPBIN_URL}/status/418`, (res) => {
let body = '';
res.on('data', (d) => {
body += d;
});
res.on('end', () => {
// Set a global variable so we have something to assert in the test
// to prove we can reach here
process.teapot = body;
// body === "I'm a teapot!"
});
})
.end();
return { message: 'ok' };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code has essentially the same effect as the "double callback" example I wrote in the PR description.

@eliangcs
Copy link
Member Author

eliangcs commented Mar 17, 2022

@xavdid sorry I haven't reviewed your PRs #512 and #511. I wanted to merge this PR to 11.x before your PRs targeting 12.x. I'll make sure to review your PRs my tomorrow! 🙇

@eliangcs
Copy link
Member Author

@xavdid can you also review/approve this PR when you're online? Thanks!

Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry! I approved the other one and I guess I missed this one in the shuffle.

I'll hold off merging mine so you have a clean path to get these out.

@eliangcs eliangcs merged commit 2a11961 into master Mar 22, 2022
@eliangcs eliangcs deleted the PDE-3038-double-callback branch March 22, 2022 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants