boost-wintls icon indicating copy to clipboard operation
boost-wintls copied to clipboard

add lowest_layer() that's also present in ssl::stream

Open DraconPern opened this issue 2 years ago • 4 comments

Added two lowest_layer that's present in ssl::stream. For example, asio\example\cpp11\ssl\client.cpp uses this function and many people also expect it. For example a post like this https://stackoverflow.com/questions/28264313/ssl-certificates-and-boost-asio

DraconPern avatar Nov 15 '21 12:11 DraconPern

I actually had the lowest_layer type some time ago, but @vinniefalco pointed out this issue in asio, so I removed it again in 36e87075fcf0ce79069668e3955d08e43108271d.

I agree that it makes sense to match the interface of the asio ssl::stream. Whether that includes bugs can of course be discussed :-)

But please have a look at the issue @vinniefalco has mentioned and let me know what you think. Thanks.

Unrelated, but I had forgotten to trigger a github actions run on pull requests, so could you please either merge or rebase this against the current master. Would just be an easy way for me to see if it works as expected. Thanks a lot.

laudrup avatar Nov 15 '21 18:11 laudrup

I think the work @vinniefalco did on beast is awesome, but regarding https://github.com/chriskohlhoff/asio/issues/251. Having a concept requirement doesn't mean the class can't have other extra functions. SyncReadStream and SyncWriteStream only requires read_some() and write_some(). As long as it has those functions the requirement is fulfilled. Also, get_executor() is a requirement for AsyncReadStream so you can't just take it out. As for the lowest_layer() function, you have to implement it. I think it's because the documentation forgot to mention that Stream needs to be of the basic_socket type (and also not have BOOST_ASIO_NO_EXTENSIONS defined). I think the confusion is the naming at template <typename Stream> class stream. Stream is actually a boost::asio::basic_socket type, not a stdio stream. They should have named it Socket. I believe it was actually referring to stream vs datagram protocol.

Looking at basic_socket.hpp, I think it might be better if it was guarded by the BOOST_ASIO_NO_EXTENSIONS. I'll update the patch.

DraconPern avatar Nov 15 '21 23:11 DraconPern

Oh! I want to note that your examples should not depend on beast. If wintls is going to be a 'drop-in' replacement, it needs to show it can be used without beast. As of now, the examples depend on beast::get_lowest_layer which is not present in asio! kinda related to this pr, so I'll mention how important it is. :)

DraconPern avatar Nov 15 '21 23:11 DraconPern

Oh! I want to note that your examples should not depend on beast. If wintls is going to be a 'drop-in' replacement, it needs to show it can be used without beast. As of now, the examples depend on beast::get_lowest_layer which is not present in asio! kinda related to this pr, so I'll mention how important it is. :)

Only the HTTPS examples depend on boost::beast. There is a simple echo client and server that don't depend on beast.

Of course async versions of the echo/client server could be added, but I don't intend to remove the beast examples.

This library sholdn't depend on beast. Unfortunately the unit tests do because the test_stream from beast is quite handy. I have considered trying to extract that class so the tests can run using only asio and not the entire boost libraries.

laudrup avatar Nov 16 '21 15:11 laudrup