1

I'm trying to implement a simple TCP server using ASIO. The main difference here is that I'm using std::unique_ptr to hold the buffers instead of raw pointers and I'm moving them inside the completion handler in order to extend their lifetime.

Here is the code and it works just fine (I guess)

void do_read() {
    auto data = std::make_unique<uint8_t[]>(1500);
    auto undesired_var = boost::asio::buffer(data.get(), 1500);
    socket_.async_read_some(
           undesired_var,
           [self = shared_from_this(), data = std::move(data)](auto ec, auto len) mutable {
                if (!ec) {
                    std::cout << "Received " << len << " bytes :)" << std::endl;
                } else {
                    std::cout << "Read error: " << ec.message() << std::endl;
                }
           }
    );
}

Running the code above, I get the following output:

/tcp_echo/cmake-build-debug/bin/tcp_server 6666
Received 9 bytes :)

Note that I created a variable called undesired_var in order to hold the boost::asio::mutable_buffer data. When I try to remove those variables by creating them directly in the socket_.async_read_some call:

void do_read() {
    auto data = std::make_unique<uint8_t[]>(1500);
    socket_.async_read_some(
            boost::asio::buffer(data.get(), 1500),
            [self = shared_from_this(), data = std::move(data)](auto ec, auto len) mutable {
            if (!ec) {
                std::cout << "Received " << len << " bytes :)" << std::endl;
            } else {
                std::cout << "Read error: " << ec.message() << std::endl;
            }
        }
    );
}

I got the following output

tcp_echo/cmake-build-debug/bin/tcp_server 6666
Read error: Bad address

It seems that in the second case my std::unique_ptr is getting destroyed prematurely, but I can figure out why. Did I miss something?

Adriel Santos
  • 653
  • 7
  • 15
  • 3
    I suppose the following does not ask the same question, but knowing the answer to it leads to knowing the answer to your question: [function parameter evaluation order](https://stackoverflow.com/questions/9566187/) *(The relevant function is `async_read_some` and the parameters are the one that involves `data.get()` and the one that involves `= std::move(data)`.)* – JaMiT Apr 29 '22 at 02:49

1 Answers1

1

Yeah it's the order in which argument expressions are evaluated.

The std::move can happen before you do .get().

Another Bug:

Besides, there seems to be a large problem here:

auto data = std::make_unique<uint8_t>(1500);

That dynamically allocates an unsigned char (uint8_t) which is initialized from the integer value 1500. The allocation has size 1 (1 byte).

auto undesired_var = boost::asio::buffer(data.get(), 1500);

You're using it as if it is 1500 bytes. Oops. You probably meant:

auto data = std::make_unique<uint8_t[]>(1500);

The type of data is now std::unique_ptr<uint8_t[], std::default_delete<uint8_t[]>> which is an entirely different template specialization

sehe
  • 374,641
  • 47
  • 450
  • 633
  • The copy is unnecessary in first version. Just: `auto const buf = data.get(); socket_.async_readsome(buf,/**/`. No overheads, yet correct code. – Red.Wave Apr 29 '22 at 08:32
  • @Red.Wave What are you talking about? I show no copy whatsoever in my answer, besides, all the code I show is to highlight a different bug with the allocation type itself. I think you might have meant to put this comment at the question? And even then, it seems clear that you don't know [the asio interface](https://www.boost.org/doc/libs/1_79_0/doc/html/boost_asio/reference/basic_stream_socket/async_read_some.html) because `data.get()` is not a valid `MutableBufferSequence` model. – sehe Apr 29 '22 at 11:05
  • (Ah, I think I get it. You assumed that `asio::buffer(data.get(), 1500)` copies. [It doesn't](https://www.boost.org/doc/libs/1_79_0/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.buffer_invalidation)) – sehe Apr 29 '22 at 11:07
  • 1
    It's been a while since I've used asio. You're right about `asio::buffer` creating a reference rather than copying the value(I missed that one). But one way or other a buffer should be prepared for an async operation. To me, one exess lightweight automatic object is not undesired. The `get` prior to move is just a pattern I use in such cases, to avoid `shared_ptr` and finish with a `unique_ptr`. This was not meant to be an answer. Had I cared more, I'd recommend to rename `undesired_var` to `buf`. – Red.Wave Apr 30 '22 at 17:38
  • 1
    "To me, one exess lightweight automatic object is not undesired" - me neither. We were just explaining the need. "undesidered_var" is from the question. – sehe Apr 30 '22 at 20:08