Make it easier to tcp connect to a remote host (with dns resolution)

[bsergean]

Hi there,

It took me a while to figure out how to do a dns resolution properly (I was using nodeAddrInfoSync which I found in the unittest, but I actually wanted addrInfoSync and give it a port number as well on top of a host).

Part of the struggle was figuring out that I need to extract the out of the result of GetAddrInfoReq operations. Here is the code snippet that works for me now.

        auto loop = uvw::Loop::getDefault();

        auto dnsRequest = loop->resource<uvw::GetAddrInfoReq>();
        auto dnsResult = dnsRequest->addrInfoSync(host, std::to_string(port));
        if (!dnsResult.first)
        {
            std::stringstream ss;
            ss << "Could not resolve host: '" << host << "'";
            spdlog::error(ss.str());
            return;
        }
        auto address = dnsResult.second->ai_addr;

        auto client = loop->resource<uvw::TCPHandle>();
        client->connect(*address);

Once I figured out I needed to use ->ai_addr, clang told me I needed to dereference address with a * to finally give ->connect what it wants.

I suggest adding an overload to the TCPHandle connect that consume the exact same type that is returned by the DNS function. Also adding such an example to the doc would help as usually folks want to connect to a remove host, not just their localhost.

// Something along those lines ?
UVW_INLINE void TCPHandle::connect(std::unique_ptr< addrinfo, Deleter > addrInfo) {
    auto address = addrInfo->ai_addr;
    connect(*address);
}

ps: I'm using your library with great fun to write toy http client and server. (https://github.com/bsergean/uvweb). I would like to add http2 support too, probably with nghttp2 (the library used by curl). I found the name uvweb in one of the issues of those repo and used it as it was clever.

Also It would be great to have SSL support in uvw btw, but it's unclear on how to do it. There is a semi-official thing called uv_ssl_t, built on top of something called uv_link.

[skypjack]

All handles and requests are returned as std::shared_ptr in general. The *Sync operations are particular because they also return the state of the request. I don't feel like adding redundant methods to the tcp class is the right way here. Instead, in C++17 you can do something like this (out of my mind):

auto loop = uvw::Loop::getDefault();
auto req = loop->resource<uvw::GetAddrInfoReq>();

if(auto [ret, addr] = req->addrInfoSync(host, std::to_string(port)); ret) {
    auto client = loop->resource<uvw::TCPHandle>();
    client->connect(*addr);
}

That is, you can use structured binding to split the returned value and use it easily.

As for SSL, uvw offers what the underlying library offers, so probably the request should be forwarded to libuv. 🙂

[bsergean]

Thanks for bringing up structured binding, I am new to C++17 and forgot about it, but it makes the code more readable.

However this isn't what's getting in the way here, the problem is that we need the extra ->ai_addr indirection. Without it users get those errors and it took me a long time to figure out the answer.

../uvweb/HttpClient.cpp:230:17: error: no matching member function for call to 'connect' client->connect(*addr);

_deps/uvw-src/src/uvw/tcp.cpp:82:28: note: candidate function template not viable: no known conversion from 'addrinfo' to 'uvw::Addr' for 1st argument
UVW_INLINE void TCPHandle::connect(Addr addr) {
                           ^
_deps/uvw-src/src/uvw/tcp.cpp:87:28: note: candidate function not viable: no known conversion from 'addrinfo' to 'const sockaddr' for 1st argument
UVW_INLINE void TCPHandle::connect(const sockaddr &addr) {
                           ^
_deps/uvw-src/src/uvw/tcp.cpp:74:28: note: candidate function template not viable: requires 2 arguments, but 1 was provided
UVW_INLINE void TCPHandle::connect(std::string ip, unsigned int port) {
                           ^

This compiles (and works)

        client->connect(*addr->ai_addr);

> On Nov 2, 2020, at 12:37 AM, Michele Caini <notifications@github.com> wrote:
> 
> 
> All handles and requests are returned as std::shared_ptr in general. The *Sync operations are particular because they also return the state of the request.
> I don't feel like adding redundant methods to the tcp class is the right way here. Instead, in C++17 you can do something like this (out of my mind):
> 
> auto loop = uvw::Loop::getDefault();
> auto req = loop->resource<uvw::GetAddrInfoReq>();
> 
> if(auto [ret, addr] = req->addrInfoSync(host, std::to_string(port)); ret) {
>     auto client = loop->resource<uvw::TCPHandle>();
>     client->connect(*addr);
> }
> That is, you can use structured binding to split the returned value and use it easily.
> 
> As for SSL, uvw offers what the underlying library offers, so probably the request should be forwarded to libuv. 🙂
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub <https://github.com/skypjack/uvw/issues/228#issuecomment-720324165>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC2O6UJAK4Y6XM5ZWDQJGQ3SNZVUVANCNFSM4THA3DTA>.
> 

[skypjack]

To be honest, I'd make this clear in the documentation if you think it's not rather than adding more and more functions to be able to chain the connect with a random sync operation. First of all, the connect doesn't need the full pair and it doesn't care at all of the first value, so it would be like passing it an extra argument that isn't used just because. Secondly, a clean interface that gets a reference to the required object is clear and safe, while I cannot really tell the same for a function that gets an eventually null pointer the ownership model of which isn't clear. Finally, we would need to duplicate the interface for that, because now you can't make it work with the resulting object of the non-sync version of the same function and it doesn't make much sense to put this object in an std::pair with a meaninglessness boolean value to be able to use it with the new API.

Any thoughts?

[bsergean]

You're right, a documentation fix is good enough. I just want to try to transform my poor experience into a small improvement for the next 'first time user' of the library.

Another place that most people look for when exploring an API is the unittest / maybe there could be an extra tcp test-case that resolves localhost to 127.0.0.1.

On Nov 7, 2020, at 4:03 AM, Michele Caini notifications@github.com wrote:

To be honest, I'd make this clear in the documentation if you think it's not rather than adding more and more functions to be able to chain the connect with a random sync operation. First of all, the connect doesn't need the full pair and it doesn't care at all of the first value, so it would be like passing it an extra argument that isn't used just because. Secondly, a clean interface that gets a reference to the required object is clear and safe, while I cannot really tell the same for a function that gets an eventually null pointer the ownership model of which isn't clear. Finally, we would need to duplicate the interface for that, because now you can't make it work with the resulting object of the non-sync version of the same function and it doesn't make much sense to put this object in an std::pair with a meaninglessness boolean value to be able to use it with the new API.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/skypjack/uvw/issues/228#issuecomment-723437717, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UJ5PHO4PW25JMRXW33SOUZSVANCNFSM4THA3DTA.

[skypjack]

I just want to try to transform my poor experience into a small improvement for the next 'first time user' of the library.

Sure. Thank you for this. Do no misunderstand my answers. If they seem rude, this wasn't my intention. 😉 I just tried to give enough context to explain why changing the function signature of the connect isn't a good idea from my point of view. Of course, I'm always open to discuss and willing to change my mind if someone has the right arguments. 🙂

[bsergean]

No worries you're making good points. I'll try to contribue a small doc or unittest PR when I get a chance.

On Nov 7, 2020, at 2:22 PM, Michele Caini notifications@github.com wrote:

I just want to try to transform my poor experience into a small improvement for the next 'first time user' of the library.

Sure. Thank you for this. Do no misunderstand my answers. If they seem rude, this wasn't my intention. 😉 I just tried to give enough context to explain why changing the function signature of the connect isn't a good idea from my point of view. Of course, I'm always open to discuss and willing to change my mind if someone has the right arguments. 🙂

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/skypjack/uvw/issues/228#issuecomment-723502525, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UKPRYDOENC6HD7MUELSOXCDLANCNFSM4THA3DTA.

[bsergean]

For the record here is what I did to get async dns working / maybe google will bring someone here one day and I'll try to make a PR ... one day to add this to the doc.

        // async DNS lookup
        auto request = loop->resource<uvw::GetAddrInfoReq>();

        request->on<uvw::ErrorEvent>([this](const uvw::ErrorEvent& errorEvent, auto& /* handle */) {
            std::cerr << "Connection error: " << errorEvent.name()) << std::endl;
        });

        request->on<uvw::AddrInfoEvent>([loop](const auto& addrInfoEvent, auto& /* handle */) {
            // couple of trial and error to figure that one out
            sockaddr addr = *(addrInfoEvent.data)->ai_addr;

            auto client = loop->resource<uvw::TCPHandle>();
            client->connect(addr);
        });

        request->addrInfo("example.com", "80");

[skypjack]

I think we can safely close this issue. Please, feel free to reopen it if I'm wrong or to submit a PR to fix the documentation in case. Thanks.

[mister-good-deal]

Hello, I think it should be nice to have an automatic DNS resolution on address that are not IP like (x.x.x.x).

That is the behavior we expect on network libraries when you connect with domain name.

Even though understood the mechanism around the AddrInfoEvent callback, maybe an utility function that returns a uvw::Addr with a sync call could do the job.

uvw::Addr getAddressFromDomainName(const std::string &domain, unsigned int port)

[skypjack]

@ZiperRom1 uvw isn't a network library. It follows libuv. It's just a wrapper. So, the first question is: does libuv offer such a functionality?

[mister-good-deal]

Yes you're right, doing specifically TCP / IP with uvw made me forgot it is a C++ event lib wrapper around libuv.

I ended up doing a custom utility function that suited my needs:

namespace Network {

    // Get readable IP address string
    void *get_in_addr(struct sockaddr *sa) {
        if (sa->sa_family == AF_INET) {
            return &(((struct sockaddr_in*)sa)->sin_addr);
        }

        return &(((struct sockaddr_in6*)sa)->sin6_addr);
    }

    uvw::Addr getAddressFromHostName(const std::shared_ptr<Loop> &loop, const std::string &hostname, unsigned int port = 80) {
        char ipAddress[INET6_ADDRSTRLEN];
        auto dnsRequest = loop->resource<uvw::GetAddrInfoReq>();
        auto dnsResult  = dnsRequest->addrInfoSync(hostname, std::to_string(port));

        if (!dnsResult.first) {
            throw std::runtime_error("Could not resolve host " + hostname);
        }

        // Get readable IP address string
        inet_ntop(dnsResult.second->ai_family, get_in_addr(dnsResult.second->ai_addr), ipAddress, sizeof ipAddress);

        return uvw::Addr { ipAddress, port };
    }
}