HPCC-34165 HTTPLIB add timeout to SSL_connect/accept

[mckellyln]

Type of change:

• This change is a bug fix (non-breaking change which fixes an issue).
• This change is a new feature (non-breaking change which adds functionality).
• This change improves the code (refactor or other change that does not change the functionality)
• This change fixes warnings (the fix does not alter the functionality or the generated code)
• This change is a breaking change (fix or feature that will cause existing behavior to change).
• This change alters the query API (existing queries will have to be recompiled)

Checklist:

• My code follows the code style of this project.
  • My code does not create any new warnings from compiler, build system, or lint.
• The commit message is properly formatted and free of typos.
  • The commit message title makes sense in a changelog, by itself.
  • The commit is signed.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly, or...
  • I have created a JIRA ticket to update the documentation.
  • Any new interfaces or exported functions are appropriately commented.
• I have read the CONTRIBUTORS document.
• The change has been fully tested:
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have checked that this change does not introduce memory leaks.
  • I have used Valgrind or similar tools to check for potential issues.
• I have given due consideration to all of the following potential concerns:
  • Scalability
  • Performance
  • Security
  • Thread-safety
  • Cloud-compatibility
  • Premature optimization
  • Existing deployed queries will not be broken
  • This change fixes the problem, not just the symptom
  • The target branch of this pull request is appropriate for such a change.
• There are no similar instances of the same problem that should be addressed
  • I have addressed them here
  • I have raised JIRA issues to address them separately
• This is a user interface / front-end modification
  • I have tested my changes in multiple modern browsers
  • The component(s) render as expected

Smoketest:

• Send notifications about my Pull Request position in Smoketest queue.
• Test my draft Pull Request.

Testing:

[github-actions]

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-34165

Jirabot Action Result:
Workflow Transition To: Merge Pending
Additional PR: #19895

[Copilot]

Pull Request Overview

Adds timeout support to SSL handshakes by making the socket non-blocking and polling until SSL_connect/SSL_accept complete or time out.

• Introduces SSL_handleWait and SSL_timedAcceptorConnect for timed SSL handshakes.
• Updates ssl_new to toggle non-blocking mode around the handshake.
• Integrates the new timed handshake in both SSLServer and SSLClient.

Comments suppressed due to low confidence (2)

system/httplib/httplib.h:5491

• [nitpick] Function name SSL_timedAcceptorConnect implies only accept but is also used for connect. Consider renaming to SSL_timedHandshake or similar to reflect its dual role.

inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)

system/httplib/httplib.h:5491

• No tests were added to verify the new handshake timeout behavior. Consider adding unit or integration tests that trigger both success and timeout paths.

inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)

[Copilot]

The enum SSLMethod is declared in the global scope and could collide; consider moving it into the detail namespace to avoid polluting the public API.

Suggested change

	enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };
	namespace detail {
	enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };
	} // namespace detail

[Copilot]

Add comments or docstrings for SSL_handleWait to clarify its behavior, parameters (especially errCode vs. sslErr), and return semantics.

Suggested change

	/**
	* Handles SSL/TLS wait conditions for read or write operations.
	*
	* This function checks the SSL error code and waits for the socket to become
	* ready for reading or writing, based on the error condition. It uses a
	* timeout mechanism to avoid indefinite blocking.
	*
	* @param ssl A pointer to the SSL object representing the connection.
	* @param sock The socket file descriptor associated with the SSL connection.
	* @param errCode The error code returned by a previous SSL operation.
	* This is used to determine the type of wait condition.
	* @param sslErr A pointer to an integer where the SSL error code will be stored.
	* This is an output parameter that provides additional error details.
	* @param remainingMs The maximum time to wait, in milliseconds, for the socket to
	* become ready for the required operation.
	* @return Returns 0 if the socket is ready, -1 if an error occurs, or
	* a positive value if the timeout expires.
	*/

[Copilot]

Pull Request Overview

Adds timeout support for SSL handshakes in HTTPLIB by switching to non-blocking I/O and polling.

• Introduces SSL_handleWait and SSL_timedAcceptorConnect for timed SSL_accept/SSL_connect
• Updates detail::ssl_new to perform non-blocking handshake and restore blocking mode
• Modifies server and client code to use the timed handshake routine

Comments suppressed due to low confidence (2)

system/httplib/httplib.h:5491

• [nitpick] The function name SSL_timedAcceptorConnect is ambiguous since it handles both accept and connect. Consider renaming it to something more general like SSL_timedHandshake.

inline int SSL_timedAcceptorConnect(SSL *ssl, socket_t sock, SSLMethod method)

system/httplib/httplib.h:5463

• New handshake timeout helpers were added but there are no tests covering these new code paths. Consider adding unit or integration tests for SSL handshake timeouts.

inline int SSL_handleWait(SSL *ssl, socket_t sock, int errCode, int *sslErr, unsigned remainingMs)

[Copilot]

The SSLMethod enum is declared in the global namespace, which can pollute the project namespace. Consider moving SSLMethod and its helper functions into the detail namespace to encapsulate implementation details.

Suggested change

	enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };
	namespace detail {
	enum SSLMethod { SSL_ACCEPT_METHOD, SSL_CONNECT_METHOD };
	} // namespace detail

[Copilot]

[nitpick] Using a hardcoded magic number for the handshake timeout makes it inflexible. Extract this into a named constant or configuration parameter so it can be adjusted without code changes.

Suggested change

	unsigned timeoutMs = 60*1000;
	unsigned timeoutMs = CPPHTTPLIB_HANDSHAKE_TIMEOUT_MS;

[mckellyln]

@jakesmith does this look correct ?
We could probably also add the same non-blocking timeout support to SSL_read() and SSL_write().

[jakesmith]

I see that httplib.h explicitly warns that they do not support non-blocking:
https://github.com/yhirose/cpp-httplib#:~:text=Important,that%20you%20want.

Should we instead be switching to something more robust?
https://github.com/microsoft/cpprestsdk - not actively maintained though, but simple, fairly close to httplib in use I think
https://github.com/boostorg/beast - more widely used and maintained, but looks like requires quite a lot of boiler plate code

[mckellyln]

Should we add this ?
Or look at alternatives ?
@asselitx thoughts ?