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.

Sign up for GitHub

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

Jump to bottom

Refactor handshake #77

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

Refactor handshake #77

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eab1d14
Move manual_auth out of the handshake loop.
jens-diewald Feb 11, 2023
66a435c
Also send leftover handshake data as client.
jens-diewald Feb 11, 2023
4ba64de
Remove unused member from async_handshake
jens-diewald Feb 11, 2023
88ce93c
Remove redundant code
jens-diewald Feb 11, 2023
b437f7a
Make async_handshake::operator() not be a coroutine
jens-diewald Feb 11, 2023
9971739
Revert "Make async_handshake::operator() not be a coroutine"
jens-diewald Feb 16, 2023
0d8b104
Make use of async_handshake being a coroutine
jens-diewald Feb 16, 2023
c6bb0b0
Simplify code and add comment
jens-diewald Feb 16, 2023
fd3aea5
Improve handling of immediately failing async operations
jens-diewald Mar 1, 2023
7243bad
Add Boost 1.80 and Boost 1.81 to CI Unittests
jens-diewald Mar 2, 2023
12aec4a
Set toolset for new boost versions and install-boost
jens-diewald Mar 8, 2023
269342b
Increase waiting time for OCSP responder in test
jens-diewald Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 33 additions & 47 deletions include/boost/wintls/detail/async_handshake.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,21 @@

#include <boost/wintls/detail/sspi_handshake.hpp>

#include <boost/asio/coroutine.hpp>

namespace boost {
namespace wintls {
namespace detail {

template <typename NextLayer>
struct async_handshake : boost::asio::coroutine {
template<typename NextLayer>
struct async_handshake {
async_handshake(NextLayer& next_layer, detail::sspi_handshake& handshake, handshake_type type)
: next_layer_(next_layer)
, handshake_(handshake)
, entry_count_(0)
, state_(state::idle) {
: next_layer_(next_layer)
, handshake_(handshake)
, entry_count_(0)
, state_(state::idle) {
handshake_(type);
}

template <typename Self>
template<typename Self>
void operator()(Self& self, boost::system::error_code ec = {}, std::size_t length = 0) {
if (ec) {
self.complete(ec);
Expand All @@ -40,7 +38,7 @@ struct async_handshake : boost::asio::coroutine {
return entry_count_ > 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to return void ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a small lambda that returns bool.
Although i do not know why this is a lambda and not a simple bool variable. Technically, even the entry_count_ member could be a bool i suppose.
Then again, looking into the source of asio::detail::composed_op, it seems that it keeps track of its invocations itself and that we could access that via asio::asio_handler_is_continuation(&self). Is that how it should be done?

};

switch(state_) {
switch (state_) {
case state::reading:
handshake_.size_read(length);
state_ = state::idle;
Expand All @@ -53,45 +51,33 @@ struct async_handshake : boost::asio::coroutine {
break;
}

detail::sspi_handshake::state handshake_state;
BOOST_ASIO_CORO_REENTER(*this) {
while((handshake_state = handshake_()) != detail::sspi_handshake::state::done) {
if (handshake_state == detail::sspi_handshake::state::data_needed) {
BOOST_ASIO_CORO_YIELD {
state_ = state::reading;
next_layer_.async_read_some(handshake_.in_buffer(), std::move(self));
}
continue;
}

if (handshake_state == detail::sspi_handshake::state::data_available) {
BOOST_ASIO_CORO_YIELD {
state_ = state::writing;
net::async_write(next_layer_, handshake_.out_buffer(), std::move(self));
}
continue;
}

if (handshake_state == detail::sspi_handshake::state::error) {
if (!is_continuation()) {
BOOST_ASIO_CORO_YIELD {
auto e = self.get_executor();
net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); });
}
}
self.complete(handshake_.last_error());
return;
}
switch ((handshake_())) {
case detail::sspi_handshake::state::data_needed: {
state_ = state::reading;
next_layer_.async_read_some(handshake_.in_buffer(), std::move(self));
return;
}

if (!is_continuation()) {
BOOST_ASIO_CORO_YIELD {
auto e = self.get_executor();
net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); });
}
case detail::sspi_handshake::state::data_available: {
state_ = state::writing;
net::async_write(next_layer_, handshake_.out_buffer(), std::move(self));
return;
}
BOOST_ASSERT(!handshake_.last_error());
handshake_.manual_auth();
case detail::sspi_handshake::state::error: {
break;
}
case detail::sspi_handshake::state::done: {
BOOST_ASSERT(!handshake_.last_error());
handshake_.manual_auth();
break;
}
}
// If this is the first call to this function, it would cause the completion handler
// (invoked by self.complete()) to be executed on the wrong executor.
// Ensure that doesn't happen by posting the completion handler instead of calling it directly.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laudrup, i have to admit, i do not fully understand this. I wondered why the code was there and found an early commit where you added it. This comment is based on your commit message then.
Can you elaborate on this a bit further? What happens if "the wrong executor" is used?

Copy link
Contributor Author

@jens-diewald jens-diewald Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the executor we want is the one associated to our stream and it seems, that async_compose does not know about this executor on construction. If we are not in the first call of this function, the exectutor should be correctly selected by the next_layer_.async_read_some or net::async_write operations.
What i do not get:

  1. If the first call to the function uses the wrong executor, why is it ever executed in the first place?
  2. Why is self.get_executor() the correct executor that we use to post the call to self? Should that not still be the wrong executor?
  3. Is there no way to tell async_compose to use the correct executor upon construction?

Copy link
Contributor Author

@jens-diewald jens-diewald Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, i think:

  1. The first call to async_compose() is actually done synchronously when async_compose is called.
  2. This is most likely wrong and should be next_layer.get_executor()
  3. I can not find any hints towards this anywhere. I feel this would still be better than 2., though.

I will try to come up with a testcase for this and possibly fix it. Either in this PR or in a new one.
If somethink actually has to be fixed, the same applies to async_read and async_shutdown, i suppose.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has been a semantic change in boost 1.81 (see here https://cppalliance.org/boost-release/2022/11/16/KlemensBoost181.html)

Basically, you got two executors involved in every op: the one of the io_object (which you can get by self.get_io_executor()) and the one of the completion token.

if you do a composed op, the first step will get invoked from the initiating function, i.e. synchronously, i.e. before the initiating function returns.

That is, when you call async_handshake and you call self.complete directly, you have an immediate completion. For callbacks that might be ok, but it generally isn't. That's especially true for read or write functions.
e.g.

stream.async_read([]{}sockect.async_read(...);});

This recursion can now potentially be infinite if the stream is in error. Note also that it's not only a problem of stack overflow, but that some completion tokens can't handle recursive completion, such as use_awaitable (i.e. coroutines).

So, we are in a situation, where we initiate the composed op, but we know in the initation how to complete, e.g. we have an error. Then we post so avoid the recursion. Before boost 1.81 you'd post to the executor of the completion, BUT that is peculiar. Let's say you have a socket working on executor1, and the completion is on executor2. Normally, any op on the socket would only complete if executor1 is running, except for some error scenarios. This is weird & inconsistent, so we post to executor1 (the io executor) and complete on executor2.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important to note that this is not wrong

template<typename Self>
  void operator()(Self& self, boost::system::error_code ec = {}, std::size_t length = 0) {
    if (ec) {
      self.complete(ec);

Because ec will never be true when initiating the op. So you'll only reenter here with an ec set after an op, which we can safely assumed complete "as if by post".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klemens-morgenstern, thanks lot for this excellent explanation of the problem and the hint regarding the change in boost 1.81!
I do understand the problem we are trying to work around here now and i understand what should be done.
Yet, i am still struggling with the interface of the composed_op and what executor we get from self.get_executor() or the new self.get_io_executor(). (Is there any documentation available on this?)
From what you explained above, i gather self.get_executor() will return the executor of the completion handler? But what if the completion handler is a simple callback?
Also, in our case, the io_object would be the stream, right? I do not see how the composed_op would know about the executor associated to the stream in the first step. Am i missing something, or do we have to tell the composed op explicitly what we consider to be the io object?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you call async_compose you pass in a set of objects:

async_compose(implementation, token, objs....);

The objs at the end will be used to determine the executors; if the token has none it'll be the same as the io-executor. If you don't spec any objecst the system_executor will be used (which you'll never want).

Copy link
Contributor Author

@jens-diewald jens-diewald Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see!
So in the wintls::stream class we should do

  auto async_handshake(xxx) { return asio::async_compose<xxx>(async_handshake{xxx}, handler, *this);  }

(or equivalently next_layer_ instead of *this) instead of currently:

  auto async_handshake(xxx) { return asio::async_compose<xxx>(async_handshake{xxx}, handler);  }

Also, if i get everything correctly, we could post to next_layer_.get_executor() in the first call, as this should do the same thing as posting to self.get_io_executor() with boost 1.81?

if (!is_continuation()) {
auto e = self.get_executor();
net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); });
} else {
self.complete(handshake_.last_error());
}
}
Expand Down