Inconsistent move in debug and release configurations while passing unique_ptr around?

Question

So I got some code handling some simple tcp sockets using the SFML Library. Thereby a socket is created under the usage of SFML capabilities and returned from a function as an rvalue reference. An organizing function then passes this socket on ( to currently only be stored ) and signals its caller whether a socket was processed or not. This however does not work as expected.

struct TcpSocket : public ::sf::TcpSocket {};

unique_ptr<TcpSocket>&& TcpListener::nonBlockingNext() 
{
    unique_ptr<TcpSocket> new_socket (new TcpSocket) ;
    listener.setBlocking(false);
    if( listener.accept(*new_socket) == ::sf::Socket::Status::Done) 
    {
        new_socket->setBlocking(false);
        std::cout << "Connection established! " << new_socket.get() << "\n";
        return std::move(new_socket);
    }
    return std::move( unique_ptr<TcpSocket>(nullptr) );
}

bool ConnectionReception::processNextIncoming()
{
    unique_ptr<TcpSocket> new_socket (listener.nonBlockingNext());
    std::cout << " and then " << new_socket.get() << "\n";
    if( !new_socket ) return false;

    processNewTcpConnection( ::std::move(new_socket) );
    return true;
}

The afore used class of TcpListener encapsulates a sf::TcpListener in composition and simply forwards its usage.

I have a simple test, that attempts a connection.

TEST(test_NetworkConnection, single_connection)
{
    ConnectionReception reception;

    reception.listen( 55555 );
    std::this_thread::sleep_for( 50ms );

    TcpSocket remote_socket;
    remote_socket.connect( "127.0.0.1", 55555 );

    std::this_thread::sleep_for( 10ms );
    EXPECT_TRUE( reception.processNextIncoming() );
}

This test fails differently in both configurations I am compiling it. In debug ( g++ -g3 ) the test is failing unexpectedly.

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from test_NetworkConnection
[ RUN      ] test_NetworkConnection.single_connection
Connection established! 0x6cf7ff0
 and then 0
test\test_NetworkConnection.cpp:24: Failure
Value of: reception.processNextIncoming()
  Actual: false
Expected: true
[  FAILED  ] test_NetworkConnection.single_connection (76 ms)
[----------] 1 test from test_NetworkConnection (78 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (87 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] test_NetworkConnection.single_connection

 1 FAILED TEST

Debugging and output shows, that the first return of nonBlockingNext(), the one that returns an accepted socket by the listener, has been reached, but in the subsequent outer function of processNextIncoming the value of new_socket is not set/is nullptr.

In Release, that is with g++ -O3 the output shows promise, but the test itself crashes with a segfault, seemingly in test-teardown, maybe when freeing sockets, which I determined through further outputs, as debugging in optimized code is not very fruitfull.

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from test_NetworkConnection
[ RUN      ] test_NetworkConnection.single_connection
Connection established! 0xfe7ff0
 and then 0xfe7ff0

I further have noticed while debugging in the -g3 compilation, that the construction of new_socket in 'nonBlockingNext()' is seemingly reached again before returning:

Thread 1 hit Breakpoint 1, network::TcpListener::nonBlockingNext (this=0x640f840)
    at test/../src/NetworkConnection.hpp:40
40          unique_ptr<TcpSocket> new_socket (new TcpSocket) ;
(gdb) n
41          listener.setBlocking(false);
(gdb)
42          if( listener.accept(*new_socket) == ::sf::Socket::Status::Done)
(gdb)
44              new_socket->setBlocking(false);
(gdb)
45              std::cout << "Connection established! " << new_socket.get() << "\n";
(gdb)
Connection established! 0x6526340
46              return std::move(new_socket);
(gdb)
40          unique_ptr<TcpSocket> new_socket (new TcpSocket) ;                <<<<<<--------- here
(gdb)
49      }
(gdb)
network::ConnectionReception::processNextIncoming (this=0x640f840) at test/../src/NetworkConnection.hpp:79
79          std::cout << " and then " << new_socket.get() << "\n";
(gdb)
 and then 0
80          if( !new_socket ) return false;
(gdb)

A step, that is most likely optimized away in a release configuration or could just be gdb being weird.

What is going wrong? How do I proceed and get this to work? Did I make any mistakes with the rvalues and moves?

Answer 1

You have undefined behavior here:

unique_ptr<TcpSocket>&& TcpListener::nonBlockingNext() 
{
    unique_ptr<TcpSocket> new_socket (new TcpSocket) ;
    //...
    if( /*...*/) 
    {
        //...
        return std::move(new_socket);
    }
    //...
}

The problem is that you are returning a reference to a local variable (new_socket). Don't be distracted by it being an rvalue reference - it is still a reference! You should return unique_ptr by value instead. And, even though it is legal to std::move() the value you are returning, it is useless at best or misses an optimization at worst - so make it just return new_socket.