Update TIdSchedulerOfThread to allow custom per-thread startup/shutdown operations

[rlebeau]

Add OnThreadStartup/Shutdown events and virtual methods to TIdSchedulerOfThread and have them be called by TIdThread.Before/AfterExecute(). This way, users can perform per-thread initializations/cleanups, such as calling CoInitialize/Ex() and CoUninitialize().

[rlebeau]

Note that per-thread logic is already achievable today, by deriving a custom class from TIdThreadWithTask and overriding its virtual (Before|After)Execute() and (Before|After)Run() methods, and then assigning a TIdSchedulerOfThread(Default|Pool) component to the server's Scheduler property and assigning the custom class type to the Scheduler's ThreadClass property. Adding new events would just make this job a little easier.

[ralfjunker]

Your suggestion does not work for TIdListenerThread, I'm afraid.

Therefore I would welcome TIdThread events, especially TIdThread.OnCleanup. This event, in particular, allows an IOHandler to clean up a thread while the thread is still running, which is a requirement for memory allocated to thread-local storage (TLS, https://en.wikipedia.org/wiki/Thread-local_storage).

[rlebeau]

An IOHandler is not tied to a thread, so an IOHandler should not be managing TLS memory at all.

[ralfjunker]

OpenSSL 1.1.1 uses TLS to manage memory, for example here: https://github.com/openssl/openssl/blob/25fa346e906c4f487727cfebd5a40740709e677b/crypto/rand/drbg_lib.c#L1108-L1123

Linking against the OpenSSL 1.1.1. DLLs, this memory is freed via DllMain.

With OpenSSL 1.1.1 static linking without DLLs, threads do not call DllMain automatically. OPENSSL_thread_stop() must be invoked manually at thread cleanup to avoid memory leaks.

Given the current, unmodified Indy code I can not figure out a hook for an IOHandler implementation to achieve this goal.

Side note: The same TLS memory leak is mentioned in this OpenSSL issue, even though in a different context.

[rlebeau]

There is no need to hook into TIdListenerThread to manage TLS memory. TIdListenerThread is used internally by TIdTCPServer for the sole purpose of accepting new client connections. There is no IOHandler activity performed in the context of those threads, so there shouldn't be any TLS memory being used. Clients are managed by TIdThreadWithTask threads instead, which TIdListenerThread spawns for each accepted client. And the TIdThreadWithTask threads can be hooked into as I described earlier, using a custom derived class assigned to the TIdSchedulerOfThread.ThreadClass property.

[ralfjunker]

There is no need to hook into TIdListenerThread to manage TLS memory. TIdListenerThread is used internally by TIdTCPServer for the sole purpose of accepting new client connections. There is no IOHandler activity performed in the context of those threads, so there shouldn't be any TLS memory being used.

The OpenSSL 1.1.1 pull request https://github.com/IndySockets/Indy/pull/299 calls SSL_accept() from within TIdListenerThread, which then allocates TLS memory. Here is the call stack:

@CRYPTO_THREAD_set_local
@RAND_DRBG_get0_master
@drbg_delete_thread_state
RAND_bytes
@ssl_fill_hello_random
dtls_raw_hello_verify_request
@tls_handle_alpn
@ossl_statem_server_post_process_message
ossl_statem_accept
ossl_statem_accept
ossl_statem_accept
SSL_do_handshake
SSL_accept
TIdOpenSSLSocketServer.Accept
TIdListenerThread.Run
TIdThread.Execute
ThreadProc
ThreadWrapper

Clients are managed by TIdThreadWithTask threads instead, which TIdListenerThread spawns for each accepted client. And the TIdThreadWithTask threads can be hooked into as I described earlier, using a custom derived class assigned to the TIdSchedulerOfThread.ThreadClass property.

Your TIdSchedulerOfThread.ThreadClass suggestion requires considerable coding in addition to setting the IOHandler. This can easily be overlooked, implemented incorrectly, and quickly lead to errors and frustration. Not very practical to Indy users.

[rlebeau]

The OpenSSL 1.1.1 pull request #299 calls SSL_accept() from within TIdListenerThread, which then allocates TLS memory.

I see the TIdOpenSSLIOHandlerServer.Accept() method has a flaw in it, It is initiating an SSL/TLS handshake but IT MUST NOT DO THAT! The SERVER will decide AFTER Accept() has exited if/when to initiate a handshake, if one is needed. Accept() MUST return a new IOHandler for the newly accepted client socket that is initially set to PassThrough=True, period. I have now reported this issue on pull request #299.

Your TIdSchedulerOfThread.ThreadClass suggestion requires considerable coding in addition to setting the IOHandler.

Not a considerable amount of coding, really. It only takes a few lines of code to derive a new class and override its virtual methods as needed, and then make 1 property assignment at runtime (since it can't be made at design-time).

Not very practical to Indy users.

Most Indy users don't need to customize Indy's threads to begin with. This is an advanced feature for those who really need it.

[ralfjunker]

I conclude that once your advice at https://github.com/IndySockets/Indy/pull/299#issuecomment-721854929 is put into practice, TIdListenerThread should no longer allocate TLS memory and it should no longer be necessary to hook into TIdListenerThread.

This is good news. Thank you @rlebeau for pointing to the root of the problem!