-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor handshake #77
base: master
Are you sure you want to change the base?
Conversation
Or rather move it to the end where it is called at most once. This resolves the TODO on sspi_handshake::state and is intended to simplify the code to allow for further refactoring. There is a small change in behavior: As a server, all communication with the client is now done before manual_auth. Before, manual_auth was called as early as possible and possibly more data was sent through the stream after that.
What the comment stated for the AcceptSecurityContext documentation holds for InitializeSecurityContext just as much.
Again this is to simplify the code. I do not currently see a reason for the function to be a coroutine. Also add a comment about posting self.complete on the first call.
} | ||
// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- If the first call to the function uses the wrong executor, why is it ever executed in the first place?
- 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? - Is there no way to tell async_compose to use the correct executor upon construction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, i think:
- The first call to async_compose() is actually done synchronously when async_compose is called.
- This is most likely wrong and should be
next_layer.get_executor()
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
@jens-diewald First of all thanks a lot for this PR. I really appreciate it. Regarding the implementation of the handshake as a coroutine I had a lot of help from the nice guys from boost::beast, namely @vinniefalco and @madmongo1. To be honest I can't remember all the details but I would rather avoid removing the coroutine part without understanding the full implications of that. We definitely don't want the async parts of the code to be blocking which is why a coroutine was needed in the first place as far as I remember. Maybe we can get @vinniefalco and/or @madmongo1 to help a bit with this? I definitely appreciate it if this could be simplified so I hope this can be merged in one way or another. Thanks once again. |
This reverts commit b437f7a.
Change the code such that posting self is only needed once. Also add a comment explaining why this is necessary.
@laudrup Whether async_handshake is implemented as a coroutine or not should not be a problem with regard to asynchronity by itself. (Of course i could have gotten something wrong.) I have then made another commit re-adding the comment regarding posting self on the first call to the function and a small simplification, so that the respective code is needed only once. |
} | ||
} | ||
|
||
// 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. | ||
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); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net::post(e, [self = std::move(self), ec, length]() mutable { self(ec, length); }); | |
net::post(self.get_io_executor(), asio::append(std::move(self), ec, length)); |
This is bad because you lose the assocated executor, allocator & cancellation slot. Use append instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint!
asio::append
is available since boost 1.80 and from your blog post you linked above, i gather that self.get_io_executor()
is available since boost 1.81.
Currently wintls tests against older boost versions also and i think it is reasonable to support older versions for now, to keep the potential testing audience larger.
Hm, so maybe we should add a distinction based on BOOST_VERSION for now? @laudrup?
@@ -40,94 +39,54 @@ struct async_handshake : boost::asio::coroutine { | |||
return entry_count_ > 1; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
See comments in post_self.hpp.
I have moved the self-posting logic into a seperate file, added a case distinction on BOOST_VERSION and added some comments. @klemens-morgenstern, if you find the time to look at this and possibly confirm that it makes sense to you, i would appreciate that a lot. @laudrup, sorry that this went somewhat off-topic from my originally intended changes. The self-posting stuff would have deserved a seperate issue. From my side, the PR could be merged like this now. (Assuming the tests go through and there are no other objections.) |
@jens-diewald So sorry for the late reply and thanks once again for your pull request. I would definitely like this to work with boost versions earlier than the latest. A somewhat new version could be required. Assuming there's tests ensuring it works on different versions then adding it to the CI is definitely the right thing to do. Thanks for that. @klemens-morgenstern Thanks a lot for your input here. It seems like you're a lot more experienced with the details of So if we get the tests to pass and @klemens-morgenstern approves then so will I, although I'll probably look through the code again a final time before merging. Thanks a lot once again. |
This failed once on the github CI. Assuming that the failure was due to high load on the test server, this increases the timeout so that hopefully this will not fail again in the future.
Hi @jens-diewald, So sorry for not having looked at this for ages. I haven't really looked at this library for quite some time. Is this still something you'd like me to merge at some point? As you might have noticed I've update the pipeline to test newer boost versions and made a new release so it might be a good time for me to revisit this. The main issue I have with the change is that I would very much like someone else to have a look at it as well. If you're still interested I'll see if I can ping some relevant people. Thanks a lot. |
@madmongo1 has said he'll have a look at this when time permits. Hope we can get this merged. |
This is intended to simplify the handshake code.
It should simplify reusing much of the handshake code in order to resolve #75 in a future PR.
I also changed a few small things i came across while working on this. All should be documented within the commit messages.
I hope i did not miss any subtleties why async_handshake was a coroutine? I could not think of a good reason with the current code.
Feedback would be much appreciated :)