TCP Send raw packet bugs/Design flaws (Unresolved)

[fredrick]

Socket::Status TcpSocket::send(const void* data, std::size_t size, std::size_t& sent)
{
    // Check the parameters
    if (!data || (size == 0))
    {
        err() << "Cannot send data over the network (no data to send)" << std::endl;
        return Error;
    }

    // Loop until every byte has been sent
    int result = 0;
    for (sent = 0; sent < size; sent += result)
    {
        // Send a chunk of data
        result = ::send(getHandle(), static_cast<const char*>(data) + sent, size - sent, flags);

        // Check for errors
        if (result < 0)
        {
            Status status = priv::SocketImpl::getErrorStatus();

            if ((status == NotReady) && sent)
                return Partial;

            return status;
        }
    }

    return Done;
}

Should be:

Socket::Status TcpSocket::send(const void* data, const std::size_t size, std::size_t& sent)
{
    // Check the parameters
    if (!data || (size == 0))
    {
        err() << "Cannot send data over the network (no data to send)" << std::endl;
        return Error;
    }

     if (sent >= size)
    {
        err() << "Cannot send data, size error"  << std::endl;
        return Error;
    }

    // Loop until every byte has been sent
    int result = 0;
    for (; sent < size; sent += result)
    {
        // Send a chunk of data
        result = ::send(getHandle(), static_cast<const char*>(data) + sent, size - sent, flags);

        // Check for errors
        if (result < 0)
        {
            Status status = priv::SocketImpl::getErrorStatus();

            if ((status == NotReady) && sent)
                return Partial;

            return status;
        }
    }

    return Done;
}

You may need to check where this "send()" is called internally and update as needed to utilize "resume"

[binary1248]

Do you even understand what the sent parameter is for? It is a non-const reference for a reason.

[fredrick]

Yes i do, and setting a ref back to value 0 in the for loop makes zero sense

[binary1248]

Yes i do, and setting a ref back to value 0 in the for loop makes zero sense

Well... I guess you don't, because sent is an output parameter and that is the only reason why a parameter would be a non-const reference.

[fredrick]

And lets say you got a partial write to socket before "not ready"... what do you then do? There is no way to finish the last bytes on the outside without shifting the data before you call  send again...

[dabbertorres]

"sent" is so you can give it a variable to store how many bytes were sent. So you can see that data.

It's acting like a second return variable.

It's not meant to use the value you give it. It doesn't even expect you to give it a value.

[fredrick]

I now understand the original intention, but for non blocking sockets the current send function has little value for "not ready" as the caller needs to make sure next time that the remaining bytes is in the "0" position, this is not a good design.

[Nexus]

What do you mean exactly? What would be good design instead?

In the Partial case, the caller knows how many bytes have been sent yet, and can pass the remaining data correspondingly... I don't see the problem.

[fredrick]

Problem is at line 225 TcpSocket.cpp (this threw me off originally) but now this is a  new bug if the current statement is the intention

[Nexus]

Can you please describe the problem clearly? And answer my question how a good design would look?

[fredrick]

Sure,

A) Bug:

Socket::Status TcpSocket::send(const void* data, std::size_t size) at line 220 when returning not ready will not return with written as it does not have a ref.

Edit: The user in this case does not really know how much was written to socket before it returned "not Ready", this function should be avoided at all cost in its current implementation, warning is not good enugh

B) Better design :

Return written size to the caller domain (lib user) for all send functions (important for non blocking only),
user can then call send again with the same stack variable until "Done".


I have forked the socket portion of the library so i no longer rely on how you fix it, just wanted to let you know that there is at least one bug in the current implementation (ignoring lack of consts)

[zsbzsb]

And lets say you got a partial write to socket before "not ready"... what do you then do? There is no way to finish the last bytes on the outside without shifting the data before you call  send again...

What? This makes no sense because if you get a partial response then you call send again with the pointer to your data just incremented to the new position.

A) Bug:

Socket::Status TcpSocket::send(const void* data, std::size_t size) at line 220 when returning not ready will not return with written as it does not have a ref.

Read the implementation again, the send() overload that doesn't include the sent overload will return Partial if only part of the data was sent. The only difference between the two functions is that one doesn't give the user the byte count that was sent. And there is no way to eliminate this function or change the signature without breaking the API.

B) Better design :

Return written size to the caller domain (lib user) for all send functions (important for non blocking only),
user can then call send again with the same stack variable until "Done".

The sent byte count can't be returned in all functions, it is done where appropriate. There is no issue here other than with your mindset.

I have forked the socket portion of the library so i no longer rely on how you fix it

Mind linking to your repo then?

[fredrick]

And lets say you got a partial write to socket before "not ready"... what do you then do? There is no way to finish the last bytes on the outside without shifting the data before you call  send again...

What? This makes no sense because if you get a partial response then you call send again with the pointer to your data just incremented to the new position.

I don't understand what you mean with "no sense", let me try to clarify :

I have already covered this but in summary: Instead of letting user deciding the pointer arithmetic and calling send again, they call send until its "Done" or break on "Error/Disconnect" with the updated ref  "sent" variable. This is cleaner and safer than casting user data structure to const char * on the outside.

A) Bug:

Socket::Status TcpSocket::send(const void* data, std::size_t size) at line 220 when returning not ready will not return with written as it does not have a ref.

Read the implementation again, the send() overload that doesn't include the sent overload will return Partial if only part of the data was sent. The only difference between the two functions is that one doesn't give the user the byte count that was sent. And there is no way to eliminate this function or change the signature without breaking the API.

I understand breaking the API is not a good solution, though that does not mean the current "send" implementation is a good design, that's why the send variant without the &sent for non blocking socket should be avoided at all costs in its current form, printing a warning and call it "good enough" is ridiculous . To maintain compatibility you can have a "secure" variant with the added prefix(es) or postfix(es) to send()

B) Better design :

Return written size to the caller domain (lib user) for all send functions (important for non blocking only),
user can then call send again with the same stack variable until "Done".

The sent byte count can't be returned in all functions, it is done where appropriate. There is no issue here other than with your mindset.

As an public library, as answered above this is not good enough from an objective position, has nothing to do with my mindset, grow up and take some responsibilities.

I have forked the socket portion of the library so i no longer rely on how you fix it

Mind linking to your repo then?

I have no link for it, its also ported to amalgamation so i am not sure how useful it is.