It feels really weird to me that SetHostname would be inside TryAddSession.

I need something to find the session in removal. Associating name with it allows me to get the string and then do lookup. It would be great if we can come up with something that allows to lookup by both IntPtr and Name.

Sure... it just feels like calling SetHostname'd be the responsibility of the caller. Doesn't it have to be done in the case where TryAddSession returns false?

This is ony only for the lookup - there is no functional difference. And I tried to hide all this inside the handle.
If we can lookup/remove the entry just from the IntPtr session, we would not need to do that. But I'm not sure if there is good way.

I put in comment and made changes to make it clear. I also moved complementing removal so both parts are done inside the SafeHandle.

The locking around _sslSessions makes sense, since you're manipulating state depending on how the dictionary performed.

But, since you're already locking it, it feels like you want a non-Concurrent dictionary.

agreed. I was also thinking about grabbing extra reference on the session. That would allow me to use ConcurrentDictionary without locking as the session would never be released in the middle.
Do you have preference/recommendation@bartonjs ?

grabbing extra reference on the session

Something like

Interop.Ssl.SslSessionUpRef(session);if (!_sslSessions.TryAdd(...)){    // Undo the upref since it's not in the dictionary    Interop.Ssl.SslSessionFree(session);}

? (Upref inside has a race condition with the cleanup in ReleaseHandle)

That would get a little weird since in the cleanup you'd need to call free twice, I think?

The fact that we wrote ConcurrentDictionary suggests that it gives better perf (on average) than manual locking... but if the code to interact with it is doing memory/lifetime management and it becomes unreadable with the gymnastics... then locking is better for maintainability. (If it's clean code and more performant, than by all means use upref+ConcurrentDictionary)

yes. and then call free twice. I'm inclined to good with the lock and better maintainability as the perf does not depend on this. This happens one in while - not even for each SSL session. I started with ConcurrentDictionary but you are right - we don't need it at this point.

This logically nested (but not indented) if is really just the latter condition. One of these two lines should be deleted.

removed 603. I probably started something I end up not needing.

I don't believe theifdef will evaluate to true on a non-portable build. (Since it's a function identifier then)

This should probably instead be based on the NEED_OPENSSL_x_y values.

Try building with./build-native.sh -portableBuild=false and see if it works as expected.

I did build on Ubuntu 16.04 with OpenSSL 1.0.1e with portableBuildtrue andfalse. And it did not fail. Same on Ubuntu 20.04 with OpenSSL 1.1.1.
Would NEED_OPENSSL_1_1 cover also 3.0? I can still change that if you prefer it.

I did build on Ubuntu 16.04 with OpenSSL 1.0.1e with portableBuild true and false. And it did not fail.

That's... really surprising. Looking at the headers over time, it looks like it was added in 1.1.0, and has always been a function (vs a macro acting like a function), so what I'd expect is

1.0.1e in non-portable:

The compile doesn't see any definition of SSL_SESSION_get0_hostname at all, turns this into#if false and removes the code.

1.1.1 in non-portable:

SSL_SESSION_get0_hostname is a function, so the preprocessor doesn't see it, so this still turns into#if false and goes away.

In both cases the compile would succeed, but I'd expect 1.1.1-direct to fail to run (remember that./build-native.sh doesn't update the testhost, so you need to manually copy over it or make it a symlink -- or just load up the compile output in a debugger and see if there's a function body or not)

Would NEED_OPENSSL_1_1 cover also 3.0?

No, you'd need#if defined NEED_OPENSSL_1_1 || defined NEED_OPENSSL_3_0

you are right. updated. It is interesting that we don't really haveany test coverage for non-portable builds, right? (aside from compilation)

Are these interesting from a logging perspective (if so, that can easily be in a future change)?

I was thinking about logging but there are conditions that make it normal. If we hit this, there should be no functional change as we simply won't do the caching & resume.