Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with thread leakage #326

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

grahamegrieve
Copy link

If you're running a general purpose http server on the internet using indy, some clients will get disconnected without closing the socket. If that happens, and if the server is holding the connection open, then the thread waiting to hear from the client will wait indefinitely. This will lead to a leak of 3 threads (one from Indy, 2 from windows) for each time this happens. Over time, the server will run out of threads.

This fix allows for the server to set a connection time out - if the client hasn't been heard from for more than x seconds (setting when the server is created), the connection will be closed by the server, and the threads won't leak.

@rlebeau
Copy link
Member

rlebeau commented Oct 22, 2020

The server will not wait indefinitely if a client fails to disconnect cleanly. The OS itself will eventually timeout internally (may take up to several minutes) and invalidate the connection, which will then cause the socket to report errors that Indy catches so it can close the socket and cleanup the owning thread. That delay can be mitigated by enabling TCP keepalives on the connection (such as via AContext.Binding.SetKeepAliveValues()), or using the IOHandler.ReadTimeout property. Also, on Windows Vista+, the default OpenSSL SSLIOHandler also enforces a hard-coded 30-second timeout if no ReadTimeOut is specified.

That being said, there is a logical difference between the time the server should wait for a new request to arrive, versus the time the server should wait for individual bytes of that request to arrive. Your proposal is not making that distinction, only handling the latter case. And, you are only applying that timeout to TIdIOHandler.ReadLn() when reading HTTP request/chunk headers, but not to TIdIOHandler.ReadStream() when reading HTTP request/chunk bodies.

I already have pending code changes for adding new KeepAliveMaxRequests and KeepAliveTimeout properties to handle the waits between requests, when HTTP keep-alives are enabled. I would suggest fine-tuning your proposal a little more when handling the per-byte waits. At the very least, apply the proposed ConnectionTimeout to IOHandler.ReadStream() as well as IOHandler.ReadLn(). Or better, assign it to the IOHandler.ReadTimeout when a new client connects. Although really, I'm not sure a new property is actually needed, since users can already assign the AContext.Connection.IOHandler.ReadTimeout in the server's OnConnect event, if they want a reading timeout used.

Also, can you please separate the HL7 changes into a separate pull request? That has nothing to do with this TIdHTTPServer issue.

@rlebeau rlebeau added Type: Enhancement Issue is proposing a new feature/enhancement Element: HTTP Issues related to HTTP handling, TIdHTTP, TIdHTTPServer, TIdHTTPProxyServer, etc labels May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Element: HTTP Issues related to HTTP handling, TIdHTTP, TIdHTTPServer, TIdHTTPProxyServer, etc Type: Enhancement Issue is proposing a new feature/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants