New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a way for partial sends over non-blocking TcpSockets to be handled properly. #796

Merged
merged 1 commit into from Mar 2, 2015

Conversation

Projects
None yet
4 participants
@binary1248
Member

binary1248 commented Feb 6, 2015

Fixes #749.

Tested on Linux. Couldn't manage to get a partial send on Windows. OS X also needs testing.

@binary1248 binary1248 self-assigned this Feb 6, 2015

@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Feb 10, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 10, 2015

Member

Do you think someone else could get a partial send under Windows and if so, what would be a possible code example?

Member

eXpl0it3r commented Feb 10, 2015

Do you think someone else could get a partial send under Windows and if so, what would be a possible code example?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 10, 2015

Member

No idea how one would go about testing this on Windows. The "standard" test example consists of sending a huge 40MiB sf::Packet and checking if it can be received intact. On Linux, the packet gets split into multiple partial sends. On Windows, it just "sends" (queues) it all at once. I even tried sending 10x 100MiB sf::Packets. It queues the whole first packet and just returns NotReady for the subsequent ones until they can entirely be queued as well.

Remembering the issues that JungleToe was having, he mentioned that they only occurred on Ubuntu, which supports the theory that partial sends only happen on Linux (OS X/Unix?) systems.

Maybe there is a reason high performance network servers all run Linux? 😏

Member

binary1248 commented Feb 10, 2015

No idea how one would go about testing this on Windows. The "standard" test example consists of sending a huge 40MiB sf::Packet and checking if it can be received intact. On Linux, the packet gets split into multiple partial sends. On Windows, it just "sends" (queues) it all at once. I even tried sending 10x 100MiB sf::Packets. It queues the whole first packet and just returns NotReady for the subsequent ones until they can entirely be queued as well.

Remembering the issues that JungleToe was having, he mentioned that they only occurred on Ubuntu, which supports the theory that partial sends only happen on Linux (OS X/Unix?) systems.

Maybe there is a reason high performance network servers all run Linux? 😏

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 11, 2015

Member

@mantognini Any ideas about OS X?

Member

eXpl0it3r commented Feb 11, 2015

@mantognini Any ideas about OS X?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 11, 2015

Member

@eXpl0it3r I haven't had a look yet into it but it's on my todo list. I'm still trying to go through my stack of mail since I'm back. ^^'

Member

mantognini commented Feb 11, 2015

@eXpl0it3r I haven't had a look yet into it but it's on my todo list. I'm still trying to go through my stack of mail since I'm back. ^^'

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 24, 2015

Member

Here is some test code:

#include <SFML/Network.hpp>
#include <iostream>

void runTcpServer(unsigned short port)
{
    sf::TcpListener listener;
    if (listener.listen(port) != sf::Socket::Done)
        return;

    sf::TcpSocket socket;
    if (listener.accept(socket) != sf::Socket::Done)
        return;

    socket.setBlocking(false);

    sf::Packet packet;

    for (int i = 0; i < 10000000; i++)
        packet << i;

    while (socket.send(packet) == sf::Socket::NotReady)
    {
    }
}

void runTcpClient(unsigned short port)
{
    sf::IpAddress server;
    do
    {
        std::cout << "Type the address or name of the server to connect to: ";
        std::cin  >> server;
    }
    while (server == sf::IpAddress::None);

    sf::TcpSocket socket;
    if (socket.connect(server, port) != sf::Socket::Done)
        return;

    sf::Packet packet;
    if (socket.receive(packet) != sf::Socket::Done)
        return;

    for (int i = 0; i < 10000000; i++)
    {
        int j;
        packet >> j;
        if (i != j)
        {
            std::cout << "Data corruption" << std::endl;
            return;
        }
    }
}

int main()
{
    const unsigned short port = 50001;

    char who;
    std::cout << "Do you want to be a server (s) or a client (c)? ";
    std::cin  >> who;

    if (who == 's')
        runTcpServer(port);
    else
        runTcpClient(port);

    std::cout << "Press enter to exit..." << std::endl;
    std::cin.ignore(10000, '\n');
    std::cin.ignore(10000, '\n');

    return 0;
}

Like I said, on Windows it doesn't matter whether you are on master or this branch, Windows just doesn't perform partial sends for some reason. On Linux it will print "Data corruption" on master and won't print anything on this branch.

Member

binary1248 commented Feb 24, 2015

Here is some test code:

#include <SFML/Network.hpp>
#include <iostream>

void runTcpServer(unsigned short port)
{
    sf::TcpListener listener;
    if (listener.listen(port) != sf::Socket::Done)
        return;

    sf::TcpSocket socket;
    if (listener.accept(socket) != sf::Socket::Done)
        return;

    socket.setBlocking(false);

    sf::Packet packet;

    for (int i = 0; i < 10000000; i++)
        packet << i;

    while (socket.send(packet) == sf::Socket::NotReady)
    {
    }
}

void runTcpClient(unsigned short port)
{
    sf::IpAddress server;
    do
    {
        std::cout << "Type the address or name of the server to connect to: ";
        std::cin  >> server;
    }
    while (server == sf::IpAddress::None);

    sf::TcpSocket socket;
    if (socket.connect(server, port) != sf::Socket::Done)
        return;

    sf::Packet packet;
    if (socket.receive(packet) != sf::Socket::Done)
        return;

    for (int i = 0; i < 10000000; i++)
    {
        int j;
        packet >> j;
        if (i != j)
        {
            std::cout << "Data corruption" << std::endl;
            return;
        }
    }
}

int main()
{
    const unsigned short port = 50001;

    char who;
    std::cout << "Do you want to be a server (s) or a client (c)? ";
    std::cin  >> who;

    if (who == 's')
        runTcpServer(port);
    else
        runTcpClient(port);

    std::cout << "Press enter to exit..." << std::endl;
    std::cin.ignore(10000, '\n');
    std::cin.ignore(10000, '\n');

    return 0;
}

Like I said, on Windows it doesn't matter whether you are on master or this branch, Windows just doesn't perform partial sends for some reason. On Linux it will print "Data corruption" on master and won't print anything on this branch.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 24, 2015

Member

Thanks for the test code. The new branch fixes things on OS X too.

Member

mantognini commented Feb 24, 2015

Thanks for the test code. The new branch fixes things on OS X too.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 24, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 24, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 24, 2015

Member

This PR adds a new public API. Has this been discussed, or do you feel like it's the only way to go about it?

Member

LaurentGomila commented Feb 24, 2015

This PR adds a new public API. Has this been discussed, or do you feel like it's the only way to go about it?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 24, 2015

Member

I've had some discussions with @binary1248 on IRC and the proper way to solve the issue would be API breaking. As such we're left with either ignoring the issue, which given that some people are quite hung up on it and the fact that it is really a bug, is not really a solution, or add another API and essentially mark the old one as deprecated. 😉
But I guess @binary1248 can give you a way better and more detailed explanation.

Member

eXpl0it3r commented Feb 24, 2015

I've had some discussions with @binary1248 on IRC and the proper way to solve the issue would be API breaking. As such we're left with either ignoring the issue, which given that some people are quite hung up on it and the fact that it is really a bug, is not really a solution, or add another API and essentially mark the old one as deprecated. 😉
But I guess @binary1248 can give you a way better and more detailed explanation.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 24, 2015

Member

Well... essentially we have 2 constraints in this situation:

  • We need to try to send the data to tell if it will be a partial send
  • We need to tell the user that it was a partial send so they can take the necessary action

What follows is that we need to provide the user with more information than the current API does, and without changing the fact that they will need to try to send first anyway. This automatically means that we need to add an additional status. In order to not have to buffer ourselves, we return how many bytes were actually sent as well, which requires a reference parameter since we already return a status.

Either we do it like the way it is in this PR, or we try to buffer the bytes that the send wasn't able to fully send in the first call. Either way, we need to return a new status so that the user knows they need to call send again, and this way we don't have to worry about exploding buffer sizes in case the user generates more data to send than their line speed can handle.

These changes don't constitute an API break, merely an ABI break. People who just use sf::Packet won't notice anything except that their code behaves properly now on UNIX systems 😉. People who use the raw byte send will be happy that they have something they can work with now.

Member

binary1248 commented Feb 24, 2015

Well... essentially we have 2 constraints in this situation:

  • We need to try to send the data to tell if it will be a partial send
  • We need to tell the user that it was a partial send so they can take the necessary action

What follows is that we need to provide the user with more information than the current API does, and without changing the fact that they will need to try to send first anyway. This automatically means that we need to add an additional status. In order to not have to buffer ourselves, we return how many bytes were actually sent as well, which requires a reference parameter since we already return a status.

Either we do it like the way it is in this PR, or we try to buffer the bytes that the send wasn't able to fully send in the first call. Either way, we need to return a new status so that the user knows they need to call send again, and this way we don't have to worry about exploding buffer sizes in case the user generates more data to send than their line speed can handle.

These changes don't constitute an API break, merely an ABI break. People who just use sf::Packet won't notice anything except that their code behaves properly now on UNIX systems 😉. People who use the raw byte send will be happy that they have something they can work with now.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 25, 2015

Member

Ok, thanks for the details.

Member

LaurentGomila commented Feb 25, 2015

Ok, thanks for the details.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 26, 2015

Member

So... everything tested/reviewed? No more issues/remarks? 😁

Member

binary1248 commented Feb 26, 2015

So... everything tested/reviewed? No more issues/remarks? 😁

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 27, 2015

Member

I already said it, but since some concerns were raised, I'll just write it again: This PR has been re-added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 27, 2015

I already said it, but since some concerns were raised, I'll just write it again: This PR has been re-added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit d790114 into master Mar 2, 2015

@eXpl0it3r eXpl0it3r added the s:accepted label Mar 2, 2015

@binary1248 binary1248 deleted the bugfix/partial_send branch Mar 2, 2015

@zsbzsb zsbzsb referenced this pull request Mar 14, 2015

Closed

Track releasing 2.3 #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment