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

boost requirement #857

Open
ghost opened this issue Sep 20, 2018 · 16 comments
Open

boost requirement #857

ghost opened this issue Sep 20, 2018 · 16 comments

Comments

@ghost
Copy link

ghost commented Sep 20, 2018

from http://github.com/glynos/cpp-netlib:

The C++ Network Library Project -- header-only, cross-platform, standards compliant networking library.

from http://cpp-netlib.org/0.12.0/index.html:

using namespace boost::network;
using namespace boost::network::http;

how can this be header only if it requires boost - can someone please clarify

@os12
Copy link

os12 commented Sep 20, 2018

It sounds like you are conflating two distinct things: header-only and self-contained.

@ghost
Copy link
Author

ghost commented Sep 20, 2018

@os12 sorry my question wasnt clear - let me rephrase:

  1. is boost required

  2. where is it documented that boost is required

@os12
Copy link

os12 commented Sep 20, 2018

Yes, Boost is required as this lib uses Boost ASIO.

@ghost
Copy link
Author

ghost commented Sep 20, 2018

@os12 - thanks

can you also point to where this requirement is noted in the CPP-NETLIB documentation?

@os12
Copy link

os12 commented Sep 20, 2018

Not sure whether it is explicitly stated... this was written (I believe) to be a part of Boost and uses it extensively (asio, spirit, algorithms, variant, etc).

@ghost
Copy link
Author

ghost commented Sep 20, 2018

i found this - not explicit but pretty close:

It is developed by people linked to the Boost community and will soon be
submitted for review into Boost
.

http://glynos.github.io/cpp-netlib

@deanberris
Copy link
Member

Why are you looking at glynos/cpp-netlib ?

@glynos It might be worth transferring the "rootness" of the project over to cpp-netlib/cpp-netlib to avoid confusion. I think GitHub has support for this sort of thing.

@deanberris
Copy link
Member

Also, we've stopped with the header-only business for a while now. If there's something in the documentation that's misleading, we should fix it.

@ghost
Copy link
Author

ghost commented Sep 21, 2018

@deanberris i think it would be helpful to make it explicit that this project requires boost. boost is a heavy dependency and people might want to avoid cpp-netlib based on that fact.

@deanberris
Copy link
Member

@cup We tried in 0.12.0 to not require boost. The documentation in 0.12.0 is wrong if it mentions using anything related to Boost. We may be in the Boost namespace but that doesn't mean we needed Boost in 0.12.0.

We've rolled that decision back in 0.13-devel because it turns out the std::future implementation didn't have a way of querying whether a future is ready. So now we're back to that situation where we're using Boost.Thread. If/when the std::future implementation is ready (or until we decide to implement something on our own) then we can remove the dependency on Boost.

If in 0.13-devel there's no mention on the dependency on Boost, then we should fix that indeed. Pull requests are definitely welcome to the documentation. However if I look at the docs hosted on the website:

https://cpp-netlib.org/0.13.0/getting_started.html

We mention that we do need Boost:

https://cpp-netlib.org/0.13.0/getting_started.html#getting-boost

cpp-netlib depends on Boost. It should work for any version of Boost above 1.58.0. If Boost is not installed on your system, the latest package can be found on the Boost web-site. The environment variable BOOST_ROOT must be defined, which must be the full path name of the top directory of the Boost distribution. Although Boost is mostly header only, applications built using cpp-netlib still requires linking with Boost.System, Boost.Date_time, and Boost.Regex.

If this is not sufficient, then please help us improve the situation.

Cheers

@ghost
Copy link
Author

ghost commented Sep 21, 2018

@deanberris perhaps i can give a fresh perspective, as someone looking on this
project with new eyes. seen here:

in both cases you are presented with this:

using namespace boost::network;
using namespace boost::network::http;

and on neither of those pages does it say that boost is a requirement. now one
could surmise that it is based on the code. However i feel an explicit remark on
those pages, even a short one, would be helpful.

@deanberris
Copy link
Member

@cup I completely agree. We should fix those moving forward (or maybe retroactively).

You can send some pull requests against the ReStructuredText documentation in https://github.com/cpp-netlib/cpp-netlib/blob/master/libs/network/doc/index.rst -- to help make this more explicit to someone new to the project.

Cheers

@xirius
Copy link

xirius commented Sep 26, 2018

@deanberris What do you mean by :

We've rolled that decision back in 0.13-devel because it turns out the std::future implementation didn't have a way of querying whether a future is ready.

Standard library does provide a way to query the state of the future!

my_future.wait_for(std::chrono::seconds(0)) == std::future_status::ready

https://en.cppreference.com/w/cpp/thread/future_status

Or do you mean something else by the std::future state ?

@deanberris
Copy link
Member

@deanberris What do you mean by :

We've rolled that decision back in 0.13-devel because it turns out the std::future implementation didn't have a way of querying whether a future is ready.

Standard library does provide a way to query the state of the future!

my_future.wait_for(std::chrono::seconds(0)) == std::future_status::ready

https://en.cppreference.com/w/cpp/thread/future_status

Or do you mean something else by the std::future state ?

This is not guaranteed to be wait-free. It's under-specified whether .wait_for(seconds(0)) will actually never block to check if the future's state is ready. If it is, then you're right we should just be able to switch to std::shared_future cleanly.

@xirius
Copy link

xirius commented Sep 26, 2018

@deanberris Yes but as far as I know boost doesn't give you that guarantee neither.
For example boost::future::get_state uses mutex and as such may also introduce scheduling and resource contention delays.

future_state::state get_state(boost::unique_lock<boost::mutex>&) const
or

future_state::state get_state() const {
    boost::lock_guard<boost::mutex> guard(this->mutex);
    ...

I don't think you can get any faster than 0 + scheduling and resource contention delays when querying for the future's state.

@deanberris
Copy link
Member

Oh, you're right! Thanks for pointing that out, @xirius!

I thought that has_value() in boost::future could actually get away with using an atomic load to check a flag to determine whether it has a value, but it seems it does actually use synchronisation to do it (which is a shame). I can see how std::future might not want to introduce a wait-free has_value() member, because that inherently implies an atomic lookup of a sort and may never be true (or always be true?) if the associated promise or packaged task is lazily filled (i.e. only gets computed when .wait() or any of the variants are called).

In that case there's hope yet, but if we want to move away from Boost, we might need to implement our own future+promise-like thing anyway -- one that attempts to use wait-free atomic operations if it's available for the platform, either through std::atomic<...> or through compiler intrinsics. What we really need is an atomic handle to some data, but doesn't have the rest of the features that std::future and boost::future provide. Since we know it's one-way communication anyway, and potentially have live references, semantically the future-like type might behave like a std::shared_ptr<std::atomic<std::variant<exception_ptr,data_ptr>>> with read-only operations, and the promise being exactly the same thing but with write-only interfaces.

I must admit it's been a while since I gave this thought, but it might actually be feasible to remove the dependency on Boost if we update the required language to C++14 (or even C++17). That's a bit of work that might be worth doing... but it's worth exploring anyway.

Cheers

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

No branches or pull requests

3 participants