Problems with asynchronous model

[vinniefalco]

NIce work, but this code has some defects. In particular, handler helpers passed to initiating functions lose the associated executor and associated allocator. For example, this code is wrong: https://github.com/sdamm/asio_dtls/blob/7eccc52df5262cf0ac83702bd827b9f9fd2d5502/include/asio/ssl/dtls/acceptor.hpp#L765

There are also other places which violate Asio preconditions. I suggest reading this composed operation tutorial: https://www.boost.org/doc/libs/1_68_0/libs/beast/doc/html/beast/using_io/writing_composed_operations.html

[sdamm]

Thanks for the report, i am working on it.

[vinniefalco]

I can answer questions and provide code review if you submit your changes as a pull request against your own repository's master or develop branch.

[sdamm]

I did a merge request for the asociated executor would be nice if you could have a look: https://github.com/sdamm/asio_dtls/pull/4

[vinniefalco]

I'm having a hard time understanding this code. It looks like you duplicated a significant amount of functionality of the asio::ip::tcp::basic_acceptor. Why did you do that? It seems to me that you could have just made them simple free functions which operate on a regular Asio acceptor and launch the composed operation in the traditional fashion. You would have to pass as parameters the two callbacks (the cookie related functions). This would greatly simplify the code by removing a lot of redundancy. Or maybe your version does something that I missed?

Also, it looks like you are type-erasing the handler here:

https://github.com/sdamm/asio_dtls/blob/255b7aef5aabbcd0a62f25f22c3a3adaabd46285/include/asio/ssl/dtls/acceptor.hpp#L27

Why the type-erasure? This is almost always a mistake. It seems like you are losing the associated allocator. Unless I am misreading it?

I left some other comments in the pull request. Keep up your efforts!

[sdamm]

The problem is, that i don't do a listen as udp sockets don't provide this. Instead it does a async_receive_from apart from that it sets the cookie_generate and verify_callbacks (this could be done before passing the socket to async_accept). Not sure the async_receive_from can be done with the basic_socket_acceptor, as it directly uses the <reactive/...>_socket_service which does listen. After the async_receive_from the cookie gets verified and the callback is called if the cookie matched (if not the client gets a HelloVerifyRequest and should try again with the correct cookie).

The handler_base is dead code so i will remove it.

[vinniefalco]

What is a DatagramSocketType? My point is that you have this dtls::acceptor class which seems to duplicate a lot of functionality of basic_acceptor. You are calling get_implementation on the socket? Will that work correctly on Windows?

I also notice a lot of duplicated functionality in dtls::context (along with some new stuff). I would really try to avoid this.

Maybe you can give me a little bit of an overview about how this all works, it would help me give a better code review.

Also, since you're using std::move it looks like you require C++11, but you have copied quite a bit of code from Asio which is designed to make C++03 work. For example:

ASIO_STATIC_CONSTANT(long, cookie_exchange = SSL_OP_COOKIE_EXCHANGE);

Personally I think requiring C++11 is fine, but if you're going to do that then you should trim back stuff which is only used for C++03 compatibility.

Function template definitions don't need the inline keyword: https://github.com/sdamm/asio_dtls/blob/255b7aef5aabbcd0a62f25f22c3a3adaabd46285/include/asio/ssl/dtls/detail/datagram_io.hpp#L335

Finally, you have a misconfigured git on one of the machines you are using, your global git config email and username are not set correctly so some commits are missing a link to the GitHub user and also are not signed (they say "Stefan Damm"): https://github.com/sdamm/asio_dtls/commits/master

[sdamm]

DatagramSocketType is a underlying transport for the async operations like a UDP socket.

Much of the Code is copied from asio classes e.g. the context shares nearly all code with the asio::ssl::context and there could be a common base class that was had all except the method enums and the constructor. There is really not more difference between the two. That's also why i kept the C++03 compatibility code that was in ASIO to have it easier to see the changes made in ASIO and incorporate them into this code. First my idea was to derive the context class from the asio one, but as the members are private i could not initialize them differently.

Here is a Summary of the changes the classes have compared to their asio counterparts (i also added some introduction to dtls to the Readme of the repository): context.hpp (this code is nearly identical)

• dtls methods (dtlsv1_client, ... constructor takes a dtls_method) impl/context.ipp (here is also little change)
• code for corresponding dtls methods added

socket.hpp

• set_mtu
• set_cookie_generate_callback
• set_cookie_verify_callback
• verify_cookie
• io done through the datagram_io (to allow the use of send instead of async_write)

acceptor.hpp

• uses async_receive_from instead of async_accept
• sets the vookie_verify callbacks on the socket
• a socket is passed into async_accept

detail/engine.hpp

• set_mtu
• generate_cookie
• do_dtls_listen
• use ssl_app_data to store cookie and verify callbacks (needed becaus there is only one pointer for user data)

The diffs between the asio_dtls and asio classes (i removed namespace, header guard and include differences): https://github.com/sdamm/asio_dtls_diff