For my own understanding: MarkPopErrorOnReturn basically means that OpenSSL errors inside this scope are swallowed/ignored?

Exactly, all errors that occur in this scope are dropped once the destructor is called (unless they have been handled specifically).

And now that we're handling the errors manually, Node will be throwing an error instead of just crashing, I suppose?

And because we're manually handling the errors now, it's better because we'll throw an error that you could catch and what not instead of just crashing, right?

It won't throw more errors than before (except for ::Init), it will just make sure that if it throws, it also clears the internal error queue of OpenSSL.

EVP_CipherInit_ex() can fail for legitimate reasons (e.g. an engine that fails to load) so a CHECK is not appropriate.

I assume the old behavior (= no error checking) isn't appropriate either?

Indeed, errors should be reported.

That would make this semver-major I guess, so I'll probably just leave the CHECKs out for now.

Mwah, I don't think better error checking constitutes semver-major. The kind of bugs it'd flush out are programmer bugs and environmental issues.

This should ideally be MarkPopErrorOnReturn. ClearErrorOnReturn is a blunt hammer and might swallow genuine errors.

Point taken, I can change it. In practice, the error stack should be empty when the JS layer calls into the C++ layer though, right?

Yes, except for the following:

1. It can mess up C++ -> C++ calls when the caller also manipulates the error stack.
2. I know of at least one add-on that leaves openssl errors on the stack for some time.

Style nit, but shouldn't this be done the other way around? Eg: EVP_CipherInit_ex(...) != 1? Would love to hear why you chose this over the former.

Didn't know we had a preference. I usually do the opposite, but I think having != 1 at the end of a multi-line expression looks weird.

Fun fact, this style was commonly used to prevent accidental assignment of variables / pointers in boolean expressions.

Because people would accidentally type = instead of == all the times? Sounds more likely in the era of butterfly keyboards, heh. If it makes sense for you to keep this, feel free to do so. I agree wholeheartedly that != 1 looks weird when in a multiline statement, just wanted to make sure we're not going against the style guide here.

Yes, even though I personally still prefer var == constant, the only reason here is that it looks weird. If no one has a strong opinion against this, I'd prefer to keep it this way.

Same as above.

Same

This is getting boring, isn't it? 😛

And now that we're handling the errors manually, Node will be throwing an error instead of just crashing, I suppose?

Same as above.

Same as above.

God, isn't this repetitive 😅

And because we're manually handling the errors now, it's better because we'll throw an error that you could catch and what not instead of just crashing, right?

I'm a bit intrigued.

https://www.openssl.org/docs/man1.0.2/crypto/EVP_EncryptInit.html doesn't show EVP_CIPHER_CTX_ctrl returning anything. If it's actually returning void (I hope not), the would this work?

If it doesn't return void, shouldn't we use an if statement here as well and throw something nice instead of crashing the process?

The only reasons I can think of right now are a bug (e.g. because we permitted the call in an invalid state) or a memory allocation failure, and those are generally not handled within our APIs.