feat: TlsSocket AsyncWriteSome and AsyncReadSome #376

kostasrim · 2025-01-31T16:27:43Z

Implement async io for TlsSocket.

add AsyncReadSome
add AsyncWriteSome
add tests
fix a bug in io::AsyncRead

Resolves #361 when ready.

Signed-off-by: kostas <[email protected]>

kostasrim · 2025-01-31T16:27:58Z

Do not review, it's not ready

kostasrim · 2025-01-31T16:28:27Z

util/fibers/epoll_socket.cc

@@ -388,6 +388,7 @@ void EpollSocket::AsyncWriteSome(const iovec* v, uint32_t len, io::AsyncProgress
  async_write_pending_ = 1;
 }

+// TODO implement async functionality
 void EpollSocket::AsyncReadSome(const iovec* v, uint32_t len, io::AsyncProgressCb cb) {


epoll AsyncRead is synchronous

Signed-off-by: kostas <[email protected]>

kostasrim · 2025-02-07T17:54:04Z

util/tls/certificates/ca-cert.pem

@@ -0,0 +1,34 @@
+-----BEGIN CERTIFICATE-----


I think 50 years will be enough for this to last 🤓 (that's the --days I used to generate it)

IMO I realized that we can generate the certificates with cmake as well. Not sure if it's worth it or not 🤔

In my opinion, the better idea is to add a custom step for generating all keys and certificates on the cmake step.
You never know who or how can use these certificates and blame authors for that.

codecov-commenter · 2025-02-07T17:56:40Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 91.07612% with 34 lines in your changes missing coverage. Please review.

Project coverage is 78.67%. Comparing base (d30de86) to head (e39ae8b).
Report is 169 commits behind head on master.

Files with missing lines	Patch %	Lines
util/tls/tls_socket.cc	81.50%	32 Missing ⚠️
util/tls/tls_socket_test.cc	99.01%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   77.60%   78.67%   +1.06%     
==========================================
  Files         103      111       +8     
  Lines        7824     9825    +2001     
==========================================
+ Hits         6072     7730    +1658     
- Misses       1752     2095     +343

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: kostas <[email protected]>

kostasrim · 2025-02-10T20:02:53Z

io/io.cc

@@ -50,6 +50,7 @@ struct AsyncReadState {

  AsyncReadState(AsyncSource* source, const iovec* v, uint32_t length)
      : arr(length), owner(source) {
+    cur = arr.data();


Ouch -- we now got unit tests though 😄

vyavdoshenko · 2025-04-04T11:15:15Z

util/tls/tls_socket.h

+
+    // Main loop
+    void Run() override;
+    virtual void CompleteAsyncReq(io::Result<size_t> result) override;


override always requires to override virtual method.
virtual keyword is optional in such cases.

Good catch! I will fix. I also need to address a few things functionality/testing wise :)

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (5)

io/io.cc: Language not supported
util/fibers/epoll_socket.cc: Language not supported
util/tls/tls_socket.cc: Language not supported
util/tls/tls_socket.h: Language not supported
util/tls/tls_socket_test.cc: Language not supported

kostasrim · 2025-04-07T14:39:29Z

util/tls/tls_socket_test.cc

+      iovec send_vec{.iov_base = &send_buf, .iov_len = payload_sz_};
+      iovec read_vec{.iov_base = &res, .iov_len = payload_sz_};
+
+      // We don't need to call ssl_renegotiate here, the first read will also negotiate the protocol


The first AsyncRead causes the read to write, so the AsyncWrite that follows can't progress and it subscribes itself to the pending list. When AsyncRead finishes the renegotiation, it wakes up the pending (the AsyncWrite) and it makes progress. We would have deadlocked otherwise. This is equivalent to Yielding when WRITE/READ in progress

romange · 2025-04-27T08:20:22Z

Can I review it @kostasrim ?

romange · 2025-04-27T08:23:54Z

util/tls/tls_socket.h

+    void CompleteAsyncReq(io::Result<size_t> result) override;
+  };
+
+  friend AsyncWriteReq;


i think it will work without friend decl

romange · 2025-04-27T08:26:29Z

util/tls/tls_socket.h

+
+  // TODO clean up the optional before we yield such that progress callback can dispatch another
+  // async operation
+  std::optional<AsyncWriteReq> async_write_req_;


most of our use with tls is synchronous, so I suggest to make them at least unique_ptr (could even be plain pointers), to reduce the size of TlsSocket for most cases.

this will also allow to move Async state definitions to cc file.

Yep that makes sense. It's 130 bytes roughly extra per object

romange · 2025-04-27T08:32:10Z

util/tls/tls_socket.cc

 void TlsSocket::AsyncReadSome(const iovec* v, uint32_t len, io::AsyncProgressCb cb) {
-  io::Result<size_t> res = ReadSome(v, len);
-  cb(res);
+  CHECK(!async_read_req_.has_value());


This is not how I would design it.
TlsEngine could already have a ready read buffer. you could return quickly by fetching it without doing any async stuff. Only if the buffer is empty it's worth dispatching the upstream read.

Well, isn't that what I do here? We don't start/submit any async operation. We construct
the object and call Run() and within this function we will read the EngineBuffer first if there are bytes available -- so we are on the fast path already, that is, we don't dispatch upstream.
As I am changing it now to a unique ptr, we now always get an allocation. I think the cost would be insignificant because the allocator will be able to satisfy that without actually requesting more memory and this is not a common path anyways.

romange · 2025-04-27T08:46:38Z

util/tls/tls_socket.cc

+  }
+}
+
+void TlsSocket::AsyncWriteReq::Run() {


why do we have while (true) here but we return in most cases?
it's hard to understand this logic - at first I thought Run is fiber-blocking.

what I am trying to understand - what is the path to the second loop?

There are no fiber blocks, everything is async and yes we need that. An example would be:

We push to the engine

We call HandleOpAsync because last_push.engine_opcode < 0 and we submit an async operation to the underline socket. Its completion handler will call Run() again.

The fiber continues doing its work while the underline async operation is in flight

The async operation completes, invokes the completion handler which calls Run() again. Our state now is MaybeSendOutputAsyncTag so we reach that branch but the condition last_push.written() > 0 is not true. Now we loop, push to the engine again and proceed.

romange · 2025-04-27T08:48:00Z

util/tls/tls_socket.cc

+        return;
+      }
+      last_push = *push_res;
+      state = AsyncWriteReq::HandleOpAsyncTag;


seems like switch case without breaks would work here

romange · 2025-04-27T08:51:06Z

util/tls/tls_socket.cc

+void TlsSocket::AsyncReadReq::Run() {
+  DCHECK_GT(len, 0u);
+
+  while (true) {


here as well, why do we need a loop?

I need to check this one

romange · 2025-04-27T08:58:50Z

In general this PR is fairly big and it is hard to reason about design choices at this point. And also do a proper code review.

I suggest to start smaller. Say, implement only the read path, while use synchronous stubs to handle Engine::NEED_WRITE states. (I would even expect that for simple hello worlds scenarios, NEED_WRITE state won't even happen.)

kostasrim · 2025-05-07T18:58:37Z

In general this PR is fairly big and it is hard to reason about design choices at this point. And also do a proper code review.

I suggest to start smaller. Say, implement only the read path, while use synchronous stubs to handle Engine::NEED_WRITE states. (I would even expect that for simple hello worlds scenarios, NEED_WRITE state won't even happen.)

I followed a similar design to what you did with async reads/writes for plain sockets. The difference is that the state machine for the tls stuff is a little bit more complicated and I find it harder to express those branches/flows async. For example, if we had coros, I would not need to do state tracking on some of the loops because once the completion handler for the inflight async operation completes, it would just call the coro handler which would resume from where it was suspended before the async operation started. Now it has to reach Run() first and drive down to its path. continuations would also help here, and I even took that route on a certain path (ofc not proper proper continuation).

feat: TlsSocket AsyncWriteSome and AsyncReadSome

64e18d0

Signed-off-by: kostas <[email protected]>

kostasrim commented Jan 31, 2025

View reviewed changes

fixes and certificates

701425a

Signed-off-by: kostas <[email protected]>

kostasrim commented Feb 7, 2025

View reviewed changes

refactor, add fixes and tests

916678c

Signed-off-by: kostas <[email protected]>

kostasrim commented Feb 10, 2025

View reviewed changes

Merge branch 'master' into kpr1

802a896

vyavdoshenko reviewed Apr 4, 2025

View reviewed changes

kostasrim added 3 commits April 7, 2025 14:46

clean up

1b28ab8

add renegotiate test

8187dbc

check in commented out code

0385a16

kostasrim requested a review from Copilot April 7, 2025 14:34

Copilot AI reviewed Apr 7, 2025

View reviewed changes

Merge branch 'master' into kpr1

acc6ac2

kostasrim changed the title ~~[do not review] feat: TlsSocket AsyncWriteSome and AsyncReadSome~~ feat: TlsSocket AsyncWriteSome and AsyncReadSome Apr 7, 2025

kostasrim marked this pull request as ready for review April 7, 2025 14:36

kostasrim commented Apr 7, 2025

View reviewed changes

kostasrim added 2 commits April 10, 2025 18:20

fix stack size blow

3cbfc7a

fix mac

e39ae8b

romange reviewed Apr 27, 2025

View reviewed changes

romange force-pushed the master branch from 9e235d3 to 20abeae Compare June 8, 2025 06:09

feat: TlsSocket AsyncWriteSome and AsyncReadSome #376

Are you sure you want to change the base?

feat: TlsSocket AsyncWriteSome and AsyncReadSome #376

Uh oh!

Conversation

kostasrim commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kostasrim commented Jan 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kostasrim Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romange commented Apr 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romange commented Apr 27, 2025

Uh oh!

kostasrim commented May 7, 2025

Uh oh!

Uh oh!

kostasrim commented Jan 31, 2025 •

edited

Loading

kostasrim Feb 7, 2025 •

edited

Loading

codecov-commenter commented Feb 7, 2025 •

edited

Loading

kostasrim Feb 10, 2025 •

edited

Loading