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

outbound: Add results property to transaction #1535

Merged
merged 2 commits into from Jul 13, 2016

Conversation

Projects
None yet
3 participants
@gramakri
Collaborator

gramakri commented Jul 12, 2016

transaction.results is used widely by many plugins to store results.
The code in connection.js creates this property in init_transaction
but similar logic is missing in outbound code.

In the future, results property should move to the transaction constructor.
But this is complicated by the fact that ResultStore requres a connection
object and doing this properly requires a (risky) refactor.

(The specific crash here was that the dkim plugin tries to sign a bounce
mail and crashes when doing a txn.results.add)

outbound: Add results property to transaction
transaction.results is used widely by many plugins to store results.
The code in connection.js creates this property in init_transaction
but similar logic is missing in outbound code.

In the future, results property should move to the transaction constructor.
But this is complicated by the fact that ResultStore requres a connection
object and doing this properly requires a (risky) refactor.

(The specific crash here was that the dkim plugin tries to sign a bounce
mail and crashes when doing a txn.results.add)
@gramakri

This comment has been minimized.

Show comment
Hide comment
@gramakri

gramakri Jul 12, 2016

Collaborator

eslint requires node 4 atleast it seems (per package.json).

Collaborator

gramakri commented Jul 12, 2016

eslint requires node 4 atleast it seems (per package.json).

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jul 13, 2016

Member

In the future, results property should move to the transaction constructor.

That would not work. There are plenty of SMTP connections that never have transactions and those have results. Not to mention, what would happen to all the results that were accumulated before the transaction started?

I'd have no problem dropping node 0.10 from our test suite.

Member

msimerson commented Jul 13, 2016

In the future, results property should move to the transaction constructor.

That would not work. There are plenty of SMTP connections that never have transactions and those have results. Not to mention, what would happen to all the results that were accumulated before the transaction started?

I'd have no problem dropping node 0.10 from our test suite.

@gramakri

This comment has been minimized.

Show comment
Hide comment
@gramakri

gramakri Jul 13, 2016

Collaborator

That would not work. There are plenty of SMTP connections that never have transactions and those have results. Not to mention, what would happen to all the results that were accumulated before the transaction started?

Ah no, my intent was not to move the existing connection.results that already stores non-transactional results. My comment was to add an extra results property to transaction. So, basically we will have two results - one in transaction and one in connection. Note that the code is already this way and I am not suggesting anything new.

Collaborator

gramakri commented Jul 13, 2016

That would not work. There are plenty of SMTP connections that never have transactions and those have results. Not to mention, what would happen to all the results that were accumulated before the transaction started?

Ah no, my intent was not to move the existing connection.results that already stores non-transactional results. My comment was to add an extra results property to transaction. So, basically we will have two results - one in transaction and one in connection. Note that the code is already this way and I am not suggesting anything new.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jul 13, 2016

Member

one in transaction and one in connection. Note that the code is already this way and I am not suggesting anything new.

Ah, I misunderstood you. You are correct that there's one result store per connection, and there's also one result store per transaction.

Member

msimerson commented Jul 13, 2016

one in transaction and one in connection. Note that the code is already this way and I am not suggesting anything new.

Ah, I misunderstood you. You are correct that there's one result store per connection, and there's also one result store per transaction.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jul 13, 2016

Member

Please also make this change so that the tests pass and we can merge this:

--- a/.travis.yml
+++ b/.travis.yml
@@ -2,7 +2,7 @@ language: node_js
 node_js:
 #   - "0.6"     # no longer supported by async
 #   - "0.8"     # no longer supported by iconv
-    - "0.10"
+#   - "0.10"
     - "4"
     - "5"
     - "6"
Member

msimerson commented Jul 13, 2016

Please also make this change so that the tests pass and we can merge this:

--- a/.travis.yml
+++ b/.travis.yml
@@ -2,7 +2,7 @@ language: node_js
 node_js:
 #   - "0.6"     # no longer supported by async
 #   - "0.8"     # no longer supported by iconv
-    - "0.10"
+#   - "0.10"
     - "4"
     - "5"
     - "6"
@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Jul 13, 2016

Member

Upstream Node.js support is being dropped for 5.x soon too, I'd be more than half tempted to drop version 5 from our automated testing as well.

Member

msimerson commented Jul 13, 2016

Upstream Node.js support is being dropped for 5.x soon too, I'd be more than half tempted to drop version 5 from our automated testing as well.

@msimerson msimerson added the Bug Fix label Jul 13, 2016

Disable travis test on 0.10
eslint does not support it anymore

@msimerson msimerson merged commit b20af7b into haraka:master Jul 13, 2016

2 checks passed

codecov/project 36.13% (+<.01%) compared to f201bf5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msimerson

This comment has been minimized.

Show comment
Hide comment
Member

msimerson commented Jul 13, 2016

thanks @gramakri

@msimerson msimerson self-assigned this Jul 13, 2016

@gramakri gramakri deleted the cloudron-io:outbound_dkim_crash branch Jul 25, 2016

@swerter

This comment has been minimized.

Show comment
Hide comment
@swerter

swerter Sep 27, 2016

Contributor

Hi
I'm having troubles with this change. I try to check all outgoing emails by rspamd and save the results in Elasticsearch. With this change, the results of outgoing emails are getting reset, which means the rspamd results are not saved in Elasticsearch. Any suggestions what to do?

Contributor

swerter commented Sep 27, 2016

Hi
I'm having troubles with this change. I try to check all outgoing emails by rspamd and save the results in Elasticsearch. With this change, the results of outgoing emails are getting reset, which means the rspamd results are not saved in Elasticsearch. Any suggestions what to do?

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Sep 27, 2016

Member

Ohhhhh, getting reset...not good. Does wrapping this line:

transaction.results = new ResultStore(connection);

with an if conditional like this fix it?

if (!transaction.results) {
    transaction.results = new ResultStore(connection);
}
Member

msimerson commented Sep 27, 2016

Ohhhhh, getting reset...not good. Does wrapping this line:

transaction.results = new ResultStore(connection);

with an if conditional like this fix it?

if (!transaction.results) {
    transaction.results = new ResultStore(connection);
}
@gramakri

This comment has been minimized.

Show comment
Hide comment
@gramakri

gramakri Sep 28, 2016

Collaborator

@baudehlo @msimerson Wondering how this can ever happen? Can a transaction object be reused?

Collaborator

gramakri commented Sep 28, 2016

@baudehlo @msimerson Wondering how this can ever happen? Can a transaction object be reused?

@swerter

This comment has been minimized.

Show comment
Hide comment
@swerter

swerter Sep 28, 2016

Contributor

@msimerson Yes, adding the if condition works, the results are now correctly stored into elasticsearch.

Contributor

swerter commented Sep 28, 2016

@msimerson Yes, adding the if condition works, the results are now correctly stored into elasticsearch.

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