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

[gramakri]

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 https://github.com/haraka/Haraka/blob/15a025f27fe2016160b376e72e51d6052ab98645/plugins/avg.js#L39 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 https://github.com/haraka/Haraka/blob/15a025f27fe2016160b376e72e51d6052ab98645/plugins/clamd.js#L160 , https://github.com/haraka/Haraka/blob/15a025f27fe2016160b376e72e51d6052ab98645/plugins/delay_deny.js#L21 (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]

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

[msimerson]

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]

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.

[baudehlo]

I think a reasonable thing for this would be for plugins.js to know which hooks have a transaction, and check it exists before running the hook.

On Thu, Nov 7, 2019 at 6:50 PM Girish Ramakrishnan notifications@github.com wrote:

Argh, I just re-read my comment. I meant stale and not stable :-) Is that the source of confusion?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/haraka/Haraka/issues/2732?email_source=notifications&email_token=AAFBWY2GJFADEWNNW7UDETTQSSSUHA5CNFSM4JKPGSQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDOHP7Q#issuecomment-551319550, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFBWY3EIPIYE22UYLBQQBDQSSSUHANCNFSM4JKPGSQA .

[msimerson]

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]

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

[msimerson]

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]

@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]

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]

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