Remove bluebird and use async/await from most of the AWS plugin #8593

juanjoDiaz · 2020-12-09T01:07:54Z

Addresses: #7171

I might keep doing the same for other modules. But I think that it's better to merge small increments than wait for a full solution rewrite.

codecov-io · 2020-12-09T09:59:42Z

Codecov Report

Merging #8593 (04234f8) into master (d0beed4) will increase coverage by 0.02%.
The diff coverage is 67.32%.

@@            Coverage Diff             @@
##           master    #8593      +/-   ##
==========================================
+ Coverage   86.94%   86.97%   +0.02%     
==========================================
  Files         252      252              
  Lines        9517     9505      -12     
==========================================
- Hits         8275     8267       -8     
+ Misses       1242     1238       -4

Impacted Files	Coverage Δ
lib/classes/Variables.js	`99.72% <ø> (ø)`
lib/plugins/aws/invokeLocal/index.js	`68.53% <55.93%> (+0.10%)`	⬆️
lib/plugins/aws/deployFunction.js	`89.16% <72.97%> (+0.18%)`	⬆️
lib/plugins/aws/logs.js	`87.03% <75.00%> (-6.97%)`	⬇️
lib/plugins/aws/deployList.js	`91.66% <86.11%> (-1.89%)`	⬇️
lib/plugins/aws/configCredentials.js	`100.00% <100.00%> (ø)`
lib/plugins/aws/invoke.js	`97.82% <100.00%> (-0.05%)`	⬇️
lib/plugins/aws/metrics.js	`100.00% <100.00%> (ø)`
lib/plugins/package/lib/zipService.js	`96.29% <0.00%> (-0.04%)`	⬇️
lib/plugins/package/lib/packageService.js	`75.45% <0.00%> (+4.86%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0beed4...04234f8. Read the comment docs.

medikoo

Thank you @juanjoDiaz !

Concerning async/away adoption, we have planned a more careful step by step approach.
Check "Proposed solution" in description of #7171

Technically first planned step is #8368, and while there were volunteers it seems stalled, so you definitely can take this on.

Also we should not invest time into refactor of old test files, as they're anyway scheduled for greater refactor into runServerless variant (and that would trash most of the existing tests code)

Anyway work you've done looks very good and I'll be happy to take it. I've spotted just two minor things. See my comments.

medikoo · 2020-12-09T14:11:48Z

lib/plugins/aws/deployList/index.js

-          .then(this.setBucketName)
-          .then(this.listDeployments),
-      'deploy:list:functions:log': () => BbPromise.bind(this).then(this.listFunctions),
+      'before:deploy:list:log': async () => this.validate(),


In single method calls we may skip async token. Sometimes it's a sync function that's invoked, and hooks accept both async and sync callbacks

medikoo · 2020-12-09T14:12:37Z

lib/plugins/aws/deployList/index.js

-      .then(this.displayFunctions);
+  async listFunctions() {
+    const funcs = await this.getFunctions();
+    const funcsVersions = await await this.getFunctionVersions(funcs);


Let's clear doubled await

medikoo · 2020-12-09T14:18:13Z

test/unit/lib/plugins/aws/deployFunction/index.test.js

@@ -20,7 +20,7 @@ describe('AwsDeployFunction', () => {
  let awsDeployFunction;
  let cryptoStub;

-  beforeEach(() => {
+  beforeEach(async () => {


This refactor in general looks good, but we should not invest time into refactor of test files, as most of them are scheduled for general refactor (check issues with test label as: #8594 - currently just for some of files they were created, but in a long run all files should be addressed)

medikoo · 2020-12-09T14:26:07Z

Additionally, as it touches sensitive parts, it'll be great if you can confirm that integration tests pass with this changes

juanjoDiaz · 2020-12-09T15:01:03Z

I've checked the proposed approach.
It could definitely work. I just did these modules since I was looking around the area.

Regarding the test, I had to do some minimal refactoring because making functions async made all error async and before there were some sync errors.
But I will not do any more refactoring of test.

I have few more AWS-related modules asyncified and I'll keep pushing them one by one to ensure that tests pass.

Are the integration tests run by GitHub actions?
Do they need real AWS credentials (i.e. do they actually deploy stacks)?

medikoo · 2020-12-10T13:54:48Z

Are the integration tests run by GitHub actions?

Yes, but only after updates are merged to master

Do they need real AWS credentials (i.e. do they actually deploy stacks)?

Yes, please check the docs: https://github.com/serverless/serverless/tree/master/test#aws-integration-tests

medikoo · 2020-12-14T12:05:01Z

@juanjoDiaz is this ready for re-review? If it's the case please re-request it (in reviewers box). Thank you!

medikoo

Thanks @juanjoDiaz for update! It looks really promising. I've noticed a few issues, and suggested some minor style improvements

medikoo · 2020-12-14T16:56:03Z

lib/plugins/aws/deployFunction.js

+
+      this.serverless.service.provider.remoteFunctionData = result;
+      return result;
+    } catch (err) {


We do not handle err, so let's maybe write it as } catch {

medikoo · 2020-12-14T17:04:24Z

lib/plugins/aws/invokeLocal/index.js

+      const contents = await fse.readFile(dockerfileCachePath);
+
+      if (contents.toString() === dockerFileContent) {
+        cacheEquals = this.checkDockerImage(imageName);


It misses await, also instead of creating cacheEquals it'll be nice to simply do:

if (await this.checkDockerImage(imageName)) return imageName;

medikoo · 2020-12-14T17:04:49Z

lib/plugins/aws/deployFunction.js


-      'deploy:function:packageFunction': () =>
+      'deploy:function:packageFunction': async () =>


Let's drop async keyword here

medikoo · 2020-12-14T17:08:18Z

lib/plugins/aws/invoke.js

-          .then(this.log),
+      'invoke:invoke': async () => {
+        await this.extendedValidate();
+        const invokationReply = await this.invoke();


Let's simply write it as this.log(await this.invoke())

medikoo · 2020-12-14T17:09:53Z

lib/plugins/aws/invokeLocal/index.js

-    return spawnExt('docker', ['version']).catch(() => {
+  async checkDockerDaemonStatus() {
+    try {
+      return spawnExt('docker', ['version']);


It misses await

When returning a promise, await is not needed. But I'll add it anyway.

In try clause it is needed, otherwise eventual rejection won't be caught

Right! 🤦‍♂️

lib/plugins/aws/invokeLocal/index.js

medikoo · 2020-12-14T17:22:02Z

lib/plugins/aws/invokeLocal/index.js

+          }
+        });
+
+        mvn.on('exit', async (code, signal) => {


This handler should not be an async function as it's result is not processed (technically that creates an orphaned promise)

I disagree on this one.

And an orphaned promise is a promise that's never resolved so stays there taking up memory and event loop cycles.

This is just a promise that we don't care about the result.
It will be invoked by the internals of mvn.on and it will be garbage collected whenever it resolved and mvn is gc'd to.

The previous code was also using a promise which result was not processed (callJavaBridge)

mvn.on('exit', (code, signal) => { if (code === 0) { this.callJavaBridge(artifactPath, className, handlerName, input).then(resolve); ); } else if (!isRejected) {

We have plenty of promises that we don't process the results, like whenever we do await fs.lstat(file) to catch if the file exists.

Does that make sense?
Or would you still like me to make it sync?

And an orphaned promise is a promise that's never resolved

It is resolved, and if rejected, it'll end as unhandled rejection

The right way to do that is as follows:

mvn.on('exit', (code, signal) => { if (code === 0) { resolve(this.callJavaBridge(artifactPath, className, handlerName, input))); } ... });

medikoo · 2020-12-14T17:25:22Z

lib/plugins/aws/logs.js

 const _ = require('lodash');
 const dayjs = require('dayjs');
 const utc = require('dayjs/plugin/utc');
 const validate = require('./lib/validate');
 const formatLambdaLogEvent = require('./utils/formatLambdaLogEvent');
+const timers = require('../../utils/timers');


Let's use timers-ext/sleep instead

I missed that.

I tried to change it but it's not trivial since it's stubbed by the tests and I'm not sure how to stub function that's directly required as main export from a module (if possible).

Also, considering that the implementation of such timer is

async function sleep(interval = 0) { await new Promise(resolve => setTimeout(resolve, interval)); }

Do you think that's worth having a dependency just for that or could we remove the dep in a separate PR?

I'm not sure how to stub function that's directly required as main export from a module (if possible).

We address such cases usually with proxyquire

Do you think that's worth having a dependency

yes, we prefer to rely on single toolkit served as external package (which is well backed by tests), instead of copying those utils in every package we find use for it

medikoo · 2020-12-14T17:29:19Z

lib/utils/timers.js

@@ -0,0 +1,9 @@
+'use strict';


Let's not introduce that (we already use timers-ext/sleep)

medikoo

Thank you @juanjoDiaz we're getting near! I have few final remarks and we should be good to go

medikoo · 2020-12-15T12:51:29Z

lib/plugins/aws/invokeLocal/index.js

+    return []
+      .concat(this.options.env || [])
+      .map(itm => itm.split('='))
+      .map(([varName, ...varValue]) => [varName, varValue.join('=')])
+      .reduce(
+        (envVarsFromOptions, [varName, varValue]) => ({
+          ...envVarsFromOptions,
+          [varName]: varValue || '',
+        }),
+        {}
+      );


Let's not use reduce here (this pattern is confusing, and this use case doesn't if it well).

Let's simply do something as:

const envVarsFromOptions = {}; for (const envOption of ...) {} return envVarsFromOptions;

medikoo · 2020-12-15T12:52:55Z

lib/plugins/aws/invokeLocal/index.js

+  async ensurePackage() {
+    if (
+      this.options['skip-package'] &&
+      (await fileExists(


This is not equivalent. if fileExists crashes we need to issue spawn. With above construct it's not done, and that introduces bug

fileExist can not crash because its implementation is:

return fse .lstat(filePath) .then(stats => stats.isFile()) .catch(() => false);

It is one of those extension methods that you have especially for this case.

That's true, still my point was that we should maintain original logic (I overlooked you replaced fs.access with fileExists).

We should keepfse.access (it does a bit more that fse.exists).

Ideally this PR should be just about async/await refactor, nothing more (let's keep different concerns separated)

Reverted to fse.access and previous logic.

medikoo

As commented. This PR should be purely about async/await refactor, nothing more.

Let's discuss any other improvements in other issue (to avoid unnecessary work, let's have all ideas confirmed in issues, before jumping to PR's)

medikoo

@juanjoDiaz thanks for update. I believe we just one minor thing left. and that should be good to take in

medikoo · 2020-12-15T16:58:23Z

lib/plugins/aws/invokeLocal/index.js

-      return fse
-        .access(
+  async ensurePackage() {
+    try {


Let's not wrap if with try (ideally only logic parts we intend to catch errors from should be wrapped with try blocks)

Done!

It's now a bit uglier because we need to duplicate the spawn but it's exactly the same logic.

It's now a bit uglier because we need to duplicate the spawn but it's exactly the same logic.

No, there was no need to duplicate it. Your approach was perfect in terms that you didn't duplicate spawn, and it'll be great to maintain that.

My point was simply, that try/catch should be within if and not wrapping it

🤦‍♂️

So much back and forth that I'm starting to miss the obvious....

Done!

medikoo

Thank you @juanjoDiaz ! That's a great improvement. Code is definitely less confusing now

juanjoDiaz force-pushed the remove_bluebird branch 3 times, most recently from 3c5f1ed to b74bc0d Compare December 9, 2020 09:57

juanjoDiaz force-pushed the remove_bluebird branch from 877d14e to 67cb0b0 Compare December 9, 2020 11:12

medikoo suggested changes Dec 9, 2020

View reviewed changes

medikoo added the refactor label Dec 9, 2020

medikoo assigned juanjoDiaz Dec 9, 2020

juanjoDiaz force-pushed the remove_bluebird branch from 2ef0db7 to 1fc6bab Compare December 9, 2020 14:55

refactor(AWS Config Credentials): Refactor to async/await

7572a7c

juanjoDiaz force-pushed the remove_bluebird branch from bf52e1a to 522dcf8 Compare December 14, 2020 14:41

juanjoDiaz added 4 commits December 14, 2020 15:48

refactor(AWS Deploy Function): Refactor to async/await

8ef4974

refactor(AWS Deploy List): Refactor to async/await

2865368

refactor(AWS Invoke): Refactor to async/await

ac94bee

refactor(AWS Invoke Local): Refactor to async/await

98d0334

juanjoDiaz force-pushed the remove_bluebird branch from 522dcf8 to 29d0afb Compare December 14, 2020 14:49

juanjoDiaz added 2 commits December 14, 2020 15:53

refactor(AWS Logs): Refactor to async/await

3531e68

refactor(AWS Metrics): Refactor to async/await

c8e72a2

juanjoDiaz force-pushed the remove_bluebird branch from 29d0afb to c8e72a2 Compare December 14, 2020 14:53

juanjoDiaz requested a review from medikoo December 14, 2020 14:59

medikoo suggested changes Dec 14, 2020

View reviewed changes

juanjoDiaz force-pushed the remove_bluebird branch from c2ae55a to 31c3f33 Compare December 15, 2020 11:24

juanjoDiaz requested a review from medikoo December 15, 2020 11:36

medikoo suggested changes Dec 15, 2020

View reviewed changes

juanjoDiaz requested a review from medikoo December 15, 2020 16:23

juanjoDiaz force-pushed the remove_bluebird branch from 31c3f33 to 08570e4 Compare December 15, 2020 16:23

juanjoDiaz force-pushed the remove_bluebird branch from 08570e4 to fa113ac Compare December 15, 2020 16:29

medikoo suggested changes Dec 15, 2020

View reviewed changes

juanjoDiaz force-pushed the remove_bluebird branch from fa113ac to cf9562a Compare December 15, 2020 16:52

juanjoDiaz requested a review from medikoo December 15, 2020 16:53

medikoo suggested changes Dec 15, 2020

View reviewed changes

juanjoDiaz force-pushed the remove_bluebird branch from cf9562a to 70e8331 Compare December 15, 2020 17:07

juanjoDiaz requested a review from medikoo December 15, 2020 17:08

refactor: Refactor according to PR suggestions

04234f8

juanjoDiaz force-pushed the remove_bluebird branch from 70e8331 to 04234f8 Compare December 15, 2020 20:24

medikoo approved these changes Dec 16, 2020

View reviewed changes

medikoo merged commit 84d423d into serverless:master Dec 16, 2020

juanjoDiaz deleted the remove_bluebird branch December 16, 2020 17:51


		'deploy:function:packageFunction': () =>
		'deploy:function:packageFunction': async () =>

Remove bluebird and use async/await from most of the AWS plugin #8593

Remove bluebird and use async/await from most of the AWS plugin #8593

Conversation

juanjoDiaz commented Dec 9, 2020

codecov-io commented Dec 9, 2020 • edited by codecov bot

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo commented Dec 9, 2020

juanjoDiaz commented Dec 9, 2020

medikoo commented Dec 10, 2020

medikoo commented Dec 14, 2020

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo Dec 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 9, 2020 •

edited by codecov bot

medikoo Dec 14, 2020 •

edited