Workflow [PR], commit [56ed698]

Summary: ❌

AI Review

Summary

This PR adds silk-backed plain and TLS Poco socket implementations, a Silk::Throttler, and focused gtests. Several earlier issues have been fixed in the current code, but the new sockets still do not preserve important Poco socket compatibility contracts around flags and nonblocking operation, and one test remains timing-dependent.

Findings

⚠️ Majors

• [src/IO/SilkFiberStreamSocketImpl.cpp:163] FiberStreamSocketImpl::receiveBytes rejects all non-zero flags, so MSG_DONTWAIT | MSG_PEEK callers such as the HTTP peer-aliveness check cannot work with this socket. The secure path forwards that same flag combination to the underlying socket before SSL processing, so SecureFiberStreamSocketImpl inherits the same incompatibility. Support at least MSG_DONTWAIT | MSG_PEEK, or ensure these socket implementations cannot be used from code paths that rely on Poco receive flags.
• [src/IO/SilkFiberStreamSocketImpl.cpp:134] The public Poco nonblocking contract is still not preserved. sendBytes waits for the fiber future and loops until all bytes are sent even after setBlocking(false), and the receive/TLS BIO paths similarly wait instead of returning EAGAIN/short progress. The new supportsExternalPolling guard protects ReadBufferFromPocoSocket and WriteBufferFromPocoSocket, but direct Poco::Net::StreamSocket users can still put the socket in nonblocking mode. Either implement the nonblocking behavior or reject setBlocking(false)/MSG_DONTWAIT at the socket boundary.
• [src/IO/tests/gtest_silk_fiber_stream_socket.cpp:216] PollAndReceiveTimeout still orders the negative poll assertion with std::this_thread::sleep_for. If the client fiber is not scheduled until after this delay, x is already queued and the preceding poll can return true. Replace the sleep with explicit coordination, for example a FiberFuture or latch signaled after the client finishes the negative poll.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions: preserve or explicitly reject the remaining unsupported Poco socket contracts, and remove the timing-dependent sleep from the new test.

@mstetsyuk what's the difference with https://github.com/ClickHouse/silk/blob/main/src/perf/fiber-http.h ?

2 changes on the silk side are required:

1. Add silk::FiberScheduler::connect (Add FiberScheduler::connect and FiberScheduler::accept silk#52), otherwise this is required to make sure that the socket is unblocking - required by this logic.
2. cancelIo should not do future->result = nullptr on cancel, as there is a race between our thread calling cancel and the kernel consuming bytes from TCP connection into our userspace buffer. Failure mode: kernel reads bytes from a TCP connection, writes them to our userspace buffer, and we set future->result = nullptr. The bytes are forever lost, so when we try to read the data again (with a higher timeout, for instance), we won't see them.

what's the difference with https://github.com/ClickHouse/silk/blob/main/src/perf/fiber-http.h ?

This PR is basically https://github.com/ClickHouse/silk/blob/main/src/perf/fiber-http.h + TLS + cosmetic changes/simplifications/minor improvements (honouring the receive/send timeout) + tests

(I was under the impression that https://github.com/ClickHouse/silk/blob/main/src/perf/fiber-http.h is just for internal perf tests in the silk library - while this PR is meant for normal production usage in our codebase)

This change is required to implement this feature: #106078

I believe this one is important. But note that the throttler function just sleeps now - need to changes this. Must return a sleep duration so fiber code can do FinerScheduler::sleep

Apparently, we can make it even simpler. It's the responsibility of the caller to supply the throttler, so I think we can just add Silk::Throttler which overrides sleep: 0790620

And if the user of this class wants to set a throttler, it's on them to use the right one.

I believe this one is important. But note that the throttler function just sleeps now - need to changes this. Must return a sleep duration so fiber code can do FinerScheduler::sleep

@vadimskipin, addressed by adding a one-liner throttler implementation that does silk::FiberScheduler::sleep and not std::this_thread::sleep.

Unit tests failure should be fixed by: #106856

Hi @mstetsyuk @nikitamikhaylov @vadimskipin — a couple of suggestions to make this PR easier for reviewers and the changelog reader.

Description (currently missing):

• Why: PR body is essentially template-only; it does not explain the Throttler sleep virtualization, the Poco BIO_METHOD injection, or the supportsExternalPolling contract enforced in setAsyncCallback.

• Suggested addition:

  Describe: (1) the new build flag USE_SILK and its glibc/x86-64-v2/liburing requirements; (2) that Throttler::sleep is now virtual so Silk::Throttler can yield the fiber instead of blocking the thread; (3) that setAsyncCallback now rejects callbacks on sockets without external-polling support (Silk fiber sockets), since they use the blocking-future model; (4) the custom BIO_METHOD injection used to route OpenSSL I/O through the fiber scheduler.

Ignore this if the current title/body is intentional.

Merged with broken CI 😿