skypjack / uvw

Header-only, event based, tiny and easy to use libuv wrapper in modern C++ - now available as also shared/static library!
MIT License
1.87k stars 211 forks source link

ErrorEvent triggered in TCP client + "echo server" example under Windows #190

Closed escherstair closed 4 years ago

escherstair commented 4 years ago

I've been trying to implement a simple application which creates a TCP client and an "echo server" and completes a message exchange between them:

I started from two main points available in uwv tests: the file main.cpp and th TEST(TCP, ReadWrite) in tcp.cpp https://github.com/skypjack/uvw/blob/8233a6e6b2dd5014185222e1ad4f4d4956830cbb/test/uvw/tcp.cpp#L18. Both of them send a message from the client to the server.

The two examples use different names for the handles (tcp/tcp/client vs server/client/socket); I decided to use the second convention, but I renamed 'socket' into 'connection'. For this reason, my example has handles named: server, client and connection.

First of all I added the handle.write() in the connection->on<uvw::DataEvent> to send back the message to the server. This generates a WriteEvent for the client, but no DataEvent is triggered in this way, neither for the client, nor for the connection. And so I thought that the client needs to start listening for the echo from the server: and so I added a read() in the client->on<uvw::WriteEvent>.

I found that in this way a DataEvent is generated for the cient.

But this created an issue: when the client calls its write() for the second time (as an example to send another message), an ErrorEvent is generated for the connection (EALREADY - connection already in progress). I enforced this behavior in the first message replacing the tryWrite() with a write() https://github.com/skypjack/uvw/blob/8233a6e6b2dd5014185222e1ad4f4d4956830cbb/test/uvw/tcp.cpp#L49 Here you can see my example.

After my investigations, it's necessary that the client calls write() once, but exactly once. Then it must call tryWrite() (it seems that write() creates the connection and tryWrite() uses it, but this is not what I can see in libuv documentation). With this dirty workaround (one write(), then tryWrite() for the client), my example seems working, but I'm worried about this strange behavior. The server can call write() every time without issues.

I see this behavior under Windows when I compile with MinGW/GCC 7.3.0

skypjack commented 4 years ago

As far as I can see, you're using only functions that map all arguments to the underlying library. It would be great to know if the same problem arises also with libuv. This would tell us quite a lot. Did you try it already?

escherstair commented 4 years ago

Not yet, but I'm going to do it as soon as possible. And I'm going to split client and server in two different applications (to see if the behavior changes).

From the architectural point of view, is the second read() the right approach to ave the client listening to the answer from the server? At the very beginning I thought that only the connection should be reading (so only the first read() necessary); but in this way no DataEvent generated for the client.

skypjack commented 4 years ago

So, I looked into the code for libuv and it seems there is a discrepancy between Windows and all other systems. This explains also why you're observing the error only on Windows and we cannot reproduce it locally.

Long story short, when you invoke read on a tcp handle, this is what happens in uvw:

void read() {
    this->invoke(&uv_read_start, this->template get<uv_stream_t>(), &this->allocCallback, &readCallback);
}

Put aside the noise, this is the important part: &uv_read_start. That is, we are invoking that function from libuv.

So, let's go deeper in the underlying library:

So far, so good. The error boils down to libuv and is due to the fact that you're invoking twice the read on the handle:

client->on<uvw::WriteEvent>([](const uvw::WriteEvent &, uvw::TCPHandle &handle) {
    std::cout << "write" << std::endl;

    handle.read();
});

The server can write any time without errors because you're not invoking read each and every time.

Probably, you can just start/stop reading when you send/receive a message rather than using try_write to get around the issue.

escherstair commented 4 years ago

Your investigation has been perfect, and it found the real origin of my issue: every time the client writes a new message to the server, a WriteEvent is generated and so a handle.read() is invoked on a handle which is already reading. This is not an issue in unix-like systems, but it is in Windows (due to the different libuv implementations). I opened this issue to libuv asking some clarification on the different implementation. Almost certainly it's there for a reason, but it can be interesting to know why.

The suggeston to start/stop reading after send/receive works, but maybe the must effective way is to call read() only once for the client (as it is for the server). This can be done moving handle.read() from client->on<uvw::WriteEvent> to client->once<uvw::ConnectEvent>.

I confirm that in this way everything works fine.

skypjack commented 4 years ago

maybe the must effective way is to call read() only once for the client (as it is for the server). This can be done moving handle.read() from client->on to client->once.

It makes perfectly sense actually. I'm closing the issue. Feel free to reopen it if you've any other doubt. :+1: