update async to v2 #1545

Merged
merged 2 commits into from Oct 3, 2016

Conversation

Projects
None yet
2 participants
@msimerson
Member

msimerson commented Jul 20, 2016

CAUTION

Changes proposed in this pull request:

Todo

  • review async.* usage
  • be confident upgrading doesn't break anything
  • merge

@msimerson msimerson added this to the Go Faster! milestone Jul 20, 2016

@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Jul 20, 2016

Collaborator

This should mostly be a great win. Especially for outbound performance on large queues. Nice.

On Jul 20, 2016, at 7:24 PM, Matt Simerson notifications@github.com wrote:

CAUTION

Changes proposed in this pull request:

update async to version 2
see caolan/async/CHANGELOG
Todo

review async.* usage
be confident upgrading doesn't break anything
merge
You can view, comment on, or merge this pull request online at:

#1545

Commit Summary

update async to v2
File Changes

M package.json (2)
Patch Links:

https://github.com/haraka/Haraka/pull/1545.patch
https://github.com/haraka/Haraka/pull/1545.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Collaborator

baudehlo commented Jul 20, 2016

This should mostly be a great win. Especially for outbound performance on large queues. Nice.

On Jul 20, 2016, at 7:24 PM, Matt Simerson notifications@github.com wrote:

CAUTION

Changes proposed in this pull request:

update async to version 2
see caolan/async/CHANGELOG
Todo

review async.* usage
be confident upgrading doesn't break anything
merge
You can view, comment on, or merge this pull request online at:

#1545

Commit Summary

update async to v2
File Changes

M package.json (2)
Patch Links:

https://github.com/haraka/Haraka/pull/1545.patch
https://github.com/haraka/Haraka/pull/1545.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Jul 27, 2016

Collaborator

as far as I can tell from a quick grep, the one place that needs fixed is plugins/dkim_sign.js#231 which needs changed to:

    async.filter(dom_hier, fs.exists, function(err, results) {
Collaborator

baudehlo commented Jul 27, 2016

as far as I can tell from a quick grep, the one place that needs fixed is plugins/dkim_sign.js#231 which needs changed to:

    async.filter(dom_hier, fs.exists, function(err, results) {
@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Aug 19, 2016

Collaborator

Here's a diff to fix dkim_sign (untested):

diff --git a/plugins/dkim_sign.js b/plugins/dkim_sign.js
index 4739214..7b9a0bb 100644
--- a/plugins/dkim_sign.js
+++ b/plugins/dkim_sign.js
@@ -170,7 +170,11 @@ exports.hook_queue_outbound = exports.hook_pre_send_trans_email = function (next
         return next();
     }

-    plugin.get_key_dir(connection, function(keydir) {
+    plugin.get_key_dir(connection, function(err, keydir) {
+        if (err) {
+            connection.logerror(plugin, err);
+            return next(DENYSOFT, "Error in getting key_dir in dkim_sign");
+        }
         var domain;
         var selector;
         var private_key;
@@ -228,9 +232,17 @@ exports.get_key_dir = function (connection, cb) {
     }
     connection.logdebug(plugin, dom_hier);

-    async.filter(dom_hier, fs.exists, function(results) {
+    async.filter(dom_hier, function (file, cb) {
+        var result;
+        try {
+            cb(null, fs.exists(file));
+        }
+        catch (e) {
+            return cb(e);
+        }
+    }, function(err, results) {
         connection.logdebug(plugin, results);
-        cb(results[0]);
+        cb(err, results ? results[0] : null);
     });
 };
Collaborator

baudehlo commented Aug 19, 2016

Here's a diff to fix dkim_sign (untested):

diff --git a/plugins/dkim_sign.js b/plugins/dkim_sign.js
index 4739214..7b9a0bb 100644
--- a/plugins/dkim_sign.js
+++ b/plugins/dkim_sign.js
@@ -170,7 +170,11 @@ exports.hook_queue_outbound = exports.hook_pre_send_trans_email = function (next
         return next();
     }

-    plugin.get_key_dir(connection, function(keydir) {
+    plugin.get_key_dir(connection, function(err, keydir) {
+        if (err) {
+            connection.logerror(plugin, err);
+            return next(DENYSOFT, "Error in getting key_dir in dkim_sign");
+        }
         var domain;
         var selector;
         var private_key;
@@ -228,9 +232,17 @@ exports.get_key_dir = function (connection, cb) {
     }
     connection.logdebug(plugin, dom_hier);

-    async.filter(dom_hier, fs.exists, function(results) {
+    async.filter(dom_hier, function (file, cb) {
+        var result;
+        try {
+            cb(null, fs.exists(file));
+        }
+        catch (e) {
+            return cb(e);
+        }
+    }, function(err, results) {
         connection.logdebug(plugin, results);
-        cb(results[0]);
+        cb(err, results ? results[0] : null);
     });
 };
@baudehlo

This comment has been minimized.

Show comment
Hide comment
@baudehlo

baudehlo Aug 19, 2016

Collaborator

Oops, nuke the var results line.

Collaborator

baudehlo commented Aug 19, 2016

Oops, nuke the var results line.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Sep 1, 2016

Member

patch added to this PR

Member

msimerson commented Sep 1, 2016

patch added to this PR

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Sep 3, 2016

Member

WARNING: merging this PR will require that anyone upgrading Haraka and using dkim_sign will have to do a 'npm install' in the Haraka install dir to upgrade async. Really, the npm install step should be done always, but in this case, it's a requirement.

Member

msimerson commented Sep 3, 2016

WARNING: merging this PR will require that anyone upgrading Haraka and using dkim_sign will have to do a 'npm install' in the Haraka install dir to upgrade async. Really, the npm install step should be done always, but in this case, it's a requirement.

@msimerson msimerson merged commit af39655 into haraka:master Oct 3, 2016

2 checks passed

codecov/project 35.99% (+0.00%) compared to 47ae1cc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@msimerson msimerson deleted the msimerson:async-2 branch Oct 3, 2016

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