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.

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

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

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

Conversation

DraconPern
Copy link

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

@laudrup
Copy link
Owner

laudrup commented Nov 15, 2021

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

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.

@DraconPern
Copy link
Author

I think the work @vinniefalco did on beast is awesome, but regarding chriskohlhoff/asio#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
Copy link
Author

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. :)

@laudrup
Copy link
Owner

laudrup commented Nov 16, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants