Feature Request - TIdServerIOHandler.Accept

[Scalmax 0]

Dear community and Respectfull Developers of Internet Direct

 

I am working on group of Windows-hosted projects [almost all GUI with VCL] in C++ Builder (Tokyo to be exact, but considering update to Aleksandria when project will be feature complete), using Clang compiler. My application delegates IO operations on RS232 (or emulator) to futures using Delphi PPL (_di_ITask implementations). Note for advanced readers - I know that this compiler is shipped with bugged version of it, I fixed it with with some C++ stuff around. Next step was to add ability to tunnel serial traffic throught TCP. I implemented it using System::Net::Socket stuff for a start. Everything is implemented as interfaces so I will be able to replace it with blocking counterparts of Indy.

 

Why do I need Indy? you may ask. Well I need TLS, which myself unfamilliar at the beginning, I managed to understand the basics by now. After some analysis I stick with sgcIndy due to TLS 1.3 support (I am using Ed448-based certs). Indy TIdTCPServer is sadly forcefully multithreaded - and I already manage my threads by my own. Therefore I dig thought some code and managed to arrive on title method - TIdServerIOHandler.Accept. TIdSocketHandle is always needed here as it is OS-wrapped listening socket itself. TIdThread parameter seems to used only to check if we further loop in order to Accept incoming client connection. TIdYarn is not needed as per comment in the method (may be nullptr or nil).

 

My proposition:

Please implement abstract Accept method with only two parameters - TIdSocketHandle and Integer timeout. Current Accept method may call it in loop and be only implemented in base class. Using so will enable users of Indy to manage their connections in their own threads. There will be other benefits as well - if you look on existing overrides of the Accept method, their loop code is the same almost everywhere.

 

Kind regards.

[Remy Lebeau 1421]

9 hours ago, Scalmax said:

My proposition:

Please implement abstract Accept method with only two parameters - TIdSocketHandle and Integer timeout.

Feature requests should be directed to Indy's GitHub issue tracker:

https://github.com/IndySockets/Indy/issues

9 hours ago, Scalmax said:

Using so will enable users of Indy to manage their connections in their own threads.

TIdServerIOHandler was never intended for that purpose, or intended to be used outside of TIdTCPServer.

Edited May 9, 2023 by Remy Lebeau

[Scalmax 0]

13 hours ago, Remy Lebeau said:

TIdServerIOHandler was never intended for that purpose, or intended to be used outside of TIdTCPServer.

 

For now I will try to use a custom descendant of TIdThread instead of TIdListenerThread. Thank you for a quick reply. If I manage to get some time, I will try to check if mentioned dependency can be loosen - I do not grasp entirety of Indy server side right now. Unless, of course, you already know that such effort is doomed to fail.

[Remy Lebeau 1421]

On 5/9/2023 at 10:04 AM, Scalmax said:

For now I will try to use a custom descendant of TIdThread instead of TIdListenerThread.

It doesn't make sense to derive from TIdListenerThread since it is tied to TIdTCPServer and TIdScheduler.  From your earlier description, it sounds like you don't even want to use TIdTCPServer at all, since you have your own threads, you just want Indy's wrappers around socket I/O, right?  If that is all you need, you could try creating TIdTCPConnection and TIdSSLIOHandlerSocketOpenSSL objects and assign your own socket to the IOHandler.Binding.Handle property.

On 5/9/2023 at 10:04 AM, Scalmax said:

If I manage to get some time, I will try to check if mentioned dependency can be loosen

I'm not saying it can't be.  I'm just saying it is a new feature that should be submitted to Indy's issue tracker.

 

On a side note: I will mention that although TIdServerIOHandlerSocket.Accept() does not use the TIdYarn parameter, TIdServerIOHandlerChain.Accept() in the SuperCore package (a dead project) did use it.

[Scalmax 0]

On 5/10/2023 at 7:57 PM, Remy Lebeau said:

It doesn't make sense to derive from TIdListenerThread since it is tied to TIdTCPServer and TIdScheduler.  From your earlier description, it sounds like you don't even want to use TIdTCPServer at all, since you have your own threads, you just want Indy's wrappers around socket I/O, right?

That was my intention from the origin. As not English native speaker, I probably not stated it clearly, sorry for that.

 

In the end, I managed to wrap around code from Indy to avoid inherent differences with my interface.

 

One thing that stuck me the most: GStack variable. If you do not create something that does TIdStack.IncUsage you will quickly find nil-related exceptions.

I personally stuck with it by manually creating TIdSocketHandle objects and calling AllocateSocket on them. I know that TIdComponent.InitComponent does the job, but, IdBuffer.Create also does it.

My first thought was: remove GStack and make TIdStack.Instance(), probably with lock free pattern  https://stackoverflow.com/a/24657705   and without slow critical section. It would be code-breaking change.

 

Anyway, it is time to stop complain, create github account, and start asking for pull requests. @Remy Lebeau you are, for lack of other words, indispensable resource of Delphi community. Big thank you for your time.

[Remy Lebeau 1421]

4 hours ago, Scalmax said:

One thing that stuck me the most: GStack variable. If you do not create something that does TIdStack.IncUsage you will quickly find nil-related exceptions.

I personally stuck with it by manually creating TIdSocketHandle objects and calling AllocateSocket on them. I know that TIdComponent.InitComponent does the job, but, IdBuffer.Create also does it.

Only components increment GStack's usage count, utility classes do not.  If you use GStack without creating any components, you are expected to call TIdStack.IncUsage()/DecUsage() directly, eg:

TIdStack.IncUsage;
// use GStack as needed...
TIdStack.DecUsage;

It doesn't hurt to call IncUsage()/DecUsage() if you are not sure whether they should be called.  Just make sure they are balanced correctly.  TIdStack is reference-counted, the 1st usage increment creates the instance, and the last usage decrement destroys it.

Quote

My first thought was: remove GStack and make TIdStack.Instance(), probably with lock free pattern  https://stackoverflow.com/a/24657705   and without slow critical section. It would be code-breaking change.

I'll consider it for a future release when more design-breaking changes are being worked on.

Quote

Anyway, it is time to stop complain, create github account, and start asking for pull requests.

Edited May 29, 2023 by Remy Lebeau