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

Should connection.transaction be checked at start of each hook? #2732

Closed
gramakri opened this issue Nov 7, 2019 · 10 comments
Closed

Should connection.transaction be checked at start of each hook? #2732

gramakri opened this issue Nov 7, 2019 · 10 comments

Comments

@gramakri
Copy link
Collaborator

gramakri commented Nov 7, 2019

On some of our servers, we have noticed sporadic crashes when accessing the connection.transaction in our plugins. It appears that this variable gets set to null when the transaction went away in the middle of an async hook.

I see code like this

if (!connection.transaction) return next();
in some places where the start of the hook itself checks if the transaction is valid.

I am wondering what is the correct fix to this is:
Style 1: Check connection.transaction at start of each hook. This means there are lot of bugs like

txn.parse_body = true;
,
const transaction = connection.transaction;
(just some random plugins i checked)

Style 2: Expect connection.transaction to be valid at start of each hook but it can become null later if the hook is async. In this case, is OK to hold on to the transaction object to avoid having to check for nullness after every async function? What i mean is:

let txn = connection.transaction;
asyncFunc1(function () {
    txn.notes.xxx = 'yyy'; // this is OK even if the transaction is gone
}
@gramakri
Copy link
Collaborator Author

gramakri commented Nov 7, 2019

I am happy to fix the docs depending on the answer.

@msimerson
Copy link
Member

Code like this if (!connection.transaction) return next(); is defensive. When someone finds that particular crasher in their logs, we add that to make the problem go away.

Is your question, should we add lots more code like that proactively? If we should, there's going to be a LOT more instances of that. Which is icky. We're a bit sloppy with checking if transaction is still valid because we (anyone serious about their Haraka) run Haraka in cluster mode and after a crasher, a new process is spawned to replace the dead one.

I think a better approach is rewriting as Promises, and then handle "the transaction is null" errors just once per hook, in the final catch. Then we aren't spraying error handling code every time we spawn an async.

@gramakri
Copy link
Collaborator Author

gramakri commented Nov 7, 2019

Our email server restarts automatically as well, which is probably why we haven't seen this issue since we have been using haraka in production for years now. I just happened to accidentally see this crash/back trace.

If I understand your answer correctly, it is indeed the case that we should check for transaction object at the start of each hook. Most plugins keeps a reference of the transaction object, so they are only operating on some stable object, if the transaction goes away. Which is fine, I guess.

Thanks for clarifying.

@gramakri gramakri closed this as completed Nov 7, 2019
@baudehlo
Copy link
Collaborator

baudehlo commented Nov 8, 2019 via email

@msimerson
Copy link
Member

msimerson commented Nov 8, 2019

and check it exists before running the hook

That would be reasonable. But it doesn't completely resolve the problem. The transaction might exist when the hook starts running. Then the plugin does something async (DB, dns, etc.) and the transaction is gone when the callback fires.

Perhaps two things would resolve this:

  1. have plugins.js assure the transaction exists before calling the hook
  2. have plugins that do async things in a transaction grab a reference to the transaction first

The first lets us get rid of a bunch of if (!connection.transaction) return next(); lines. The latter avoids a persistent source of crashing.

@gramakri
Copy link
Collaborator Author

gramakri commented Nov 8, 2019

@msimerson Yes, indeed that's a great solution.

@gramakri gramakri reopened this Nov 8, 2019
@haraka haraka deleted a comment from baudehlo Dec 16, 2020
@haraka haraka deleted a comment from gramakri Dec 16, 2020
@haraka haraka deleted a comment from gramakri Dec 16, 2020
@msimerson
Copy link
Member

We have two good ways to improve upon the status quo:

  1. have async plugins grab a ref, const txn = connection.transaction
  2. use optional chaining when accessing

@MuraraAllan
Copy link
Contributor

MuraraAllan commented Dec 22, 2021

@msimerson Hey \o I'm wondering here if we want to check for falsy or for null ( undefined included )?

Can I suggest optional chaining and getting ride of falsy check (!)?

While reading the issue, "remove the falsy check ( ! )" was the first thing that came to my mind.

connection == null && connection.transaction == null ? (TS bug bited me)
optionally chained as connection?.transaction == null

I have been avoiding falsy for readability and even compatibility reasons, inclined by typescript.
It also appears to look straightforward a guard for a null, as it assertively says "if the computation results in null", instead of "if the opposite of the next computation".

I'm also available to grab the task, whichever decision you believe is best.

@msimerson
Copy link
Member

The only problem with optional chaining is that it requires node >= 14. At present, we still support node 12. We anticipate dropping support for node 12 when it goes EOL upstream (2022-04-30). However, since I recently cut a release and don't anticipate another before node 12 goes EOL, I'm open to accepting optional chaining as a solution. Just make sure to bump the min node version in package.json with your PR.

@msimerson
Copy link
Member

Closing as a largely solved problem. The solutions outlined herein are largely utilized where necessary.

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

No branches or pull requests

4 participants