Czy ten kod tworzący serwer jest poprawny?

0

Witam, analizuję pewien kod, który tworzy serwer tcp i chciałbym spytać czy nie ma tu ryzyka wycieku pamięci i czy ogólnie jest optymalnie zrobiony, bo działać to działa. W kodzie zaznaczyłem jeszcze miejsca w których nie wszystko jest dla mnie jasne.
Server.h

#ifndef SERVER_H
#define SERVER_H

#include <QTcpServer>

class Socket;

class Server : public QTcpServer
{
public:
    Server(QObject *parent = nullptr);
    bool startServer(quint16 port);
protected:
    void incomingConnection(qintptr handle);
private:
    QList<Socket*>mSockets;

};

#endif // SERVER_H

Server.cpp

#include "Server.h"
#include "Socket.h"
#include<QDebug>
#include<QTextStream>

Server::Server(QObject *parent) : QTcpServer(parent)
{

}

bool Server::startServer(quint16 port)
{
    return listen(QHostAddress::Any, port);
}

void Server::incomingConnection(qintptr handle)
{
    qDebug() << "Client connected with a handle: " << handle;
    auto socket = new Socket(handle, this);                                 //1. czy tutaj nie ma ryzyka wycieku pamięci?
    mSockets << socket;

    for(auto i : mSockets)
    {
        QTextStream T(i);
        T << "Server: Connected" << handle;
        i->flush();
    }

    connect(socket, &Socket::mReadyRead, [&](Socket *S)
    {
        qDebug() << "mReadyRead";
        QTextStream T(S);
        auto text = T.readAll();

        for(auto i : mSockets)
        {
            QTextStream K(i);
            K << text;
            i->flush();
        }       
    });

    connect(socket, &Socket::mStateChanged, [&](Socket *S, int ST)    //2. tutaj wywołuje się ciało tego connect, gdy emitowany jest sygnał mStateChanged tak?
    {                                                                                                          //3. i co to za zapis [&](Socket *S, int ST) ? Bo pierwszy raz z takim się spotykam
        qDebug() << "mStateChanged with a handle: " << S->socketDescriptor();
        if(ST == QTcpSocket::UnconnectedState)
        {
            qDebug() << "Unconnected state with a handle: " << S->socketDescriptor();
            mSockets.removeOne(S);

            for(auto i : mSockets)
            {
                QTextStream K(i);
                K << "Server: Client " << S->socketDescriptor() << " disconnected";
                i->flush();
            }
        }
    });
}

Socket.h

#ifndef SOCKET_H
#define SOCKET_H

#include <QTcpSocket>

class Socket : public QTcpSocket
{
    Q_OBJECT
public:
    Socket(qintptr handle, QObject *parent = nullptr);
signals:
    void mReadyRead(Socket*);
    void mStateChanged(Socket*, int);
};

#endif // SOCKET_H

Socket.cpp

#include "Socket.h"

Socket::Socket(qintptr handle, QObject *parent) : QTcpSocket(parent)
{
    setSocketDescriptor(handle);

    connect(this, &Socket::readyRead, [&]() {                                             //4. co to za zapis [&]() ?
        emit mReadyRead(this);   
    });
    connect(this, &Socket::stateChanged, [&](int S) {
        emit mStateChanged(this, S);
    });
}
1
  1. Czemu tak myślisz? Socket dziedziczy po QTcpSocket, wiec ma zapewnione automatyczne zwolnienie pamięci.

3 i 4.:

connect(this, &Socket::readyRead, [&]() {                                             //4. co to za zapis [&]() ?
        emit mReadyRead(this);   
    });

to jest Lambda. Przechwytujesz przez referencję wszystkie argumenty znajdujące się w tym samym bloku co lambda.

2

auto socket = new Socket(handle, this); //1. czy tutaj nie ma ryzyka wycieku pamięci?
Wygląda toto (z doświadczenia, bez oglądania dokumentacji) na mechanizm QT. Zgaduję, że ten this w wywołaniu konstruktora mówi "klasa spod tego adresu ma po mnie posprzątać". W gołym C++ nie byłaby to może dobra praktyka, ale w QT jest to tak zorganizowane i zadziała: zwróć uwagę na dziedziczenie class Server : public QTcpServer

1

Po co dziedziczenie po QTcpServer i QTcpSocket?
Zawieranie jest o wiele lepszym rozwiązaniem w tym wypadku.

0

@Tenonymous: @alagner
Tak właśnie myślałem, ale wolałem zapytać. Czyli dziedziczenie po QTcpSocket zapewnia sprzątanie po sobie? Ten "this" o tym mówi? I działa tak to z dziedziczeniem po każdej klasie Qt, że mogę bez obaw używać operatora new i nie przejmować się zwalnianiem pamięci, bo zostanie zrobione to automatycznie?

@MarekR22
To nie jest mój kod, analizuję go, bo chcę coś sieciowego napisać a nie znam się na tym, więc nie wiem czy wszystko co tutaj jest dodane jest konieczne. Zapytałem właśnie czy kod jest napisany optymalnie i czy nie ma tutaj jakichś niepotrzebnych rzeczy.

1

MarekR22 ma rację, w przypadku QTcpServer nie ma sensu dziedziczenie, o ile nie robisz czegoś dziwnego. Powinieneś obsłużyć sygnał newConnection() i użyć nextPendingConnection().

1

Jeszcze dodam, że używanie QTextStream na sokecie jest dość ryzykowne.
W 99% zadziała zgodnie z oczekiwaniem, ale nie zdziwiłbym się jeśli nie wyskoczą jakieś niespodzianki związane lokalizacją, albo faktem, że dane przez socket nie muszą przychodzić tak samo "poszatkowane" jak zostały wysłane.
QDataStream jest bardziej bezpiecznym rozwiązaniem.

Poza tym ta pętal jest bezsensu:

    for(auto i : mSockets)
    {
        QTextStream T(i);
        T << "Server: Connected" << handle;
        i->flush();
    }

Ta pętla przy odebraniu nowego połączenia opróżnia bufory wszystkich otwartych soketów, nie widzę logicznego uzasadnienia dla takiej czynności.
To samo podczas zamykania socketa.
A to, że rozsyłana jest wiadomość do wszystkich o nawiązaniu połączania, to nie jest miejsce na tą logikę. To się powinno dobywać gdzie indziej w innej klasie.

Co do pamięci nie widzę nic złego.

Jeśli kod pisał ktoś inny podejdź do niego z dużą dozą sceptycyzmu, bo trochę to pachnie programowaniem permutacyjnym.

1

Pamiętaj o jednej rzeczy, qt ma relację rodzic-dziecko
http://doc.qt.io/qt-5/objecttrees.html
Czy można ręcznie alokować i potem ręcznie zwalniać można(bez podawania rodzica)? Można.
Czy można używać smart pointerów? Można ale:
https://stackoverflow.com/questions/34433435/why-dont-the-official-qt-examples-and-tutorials-use-smart-pointers

0

Dziękuję wszystkim za pomoc. Napisałem coś takiego sugerując się też na przykładzie z oficjalnej strony qt oraz kilku innych, ale w ten sposób robię dziedziczenie po QTcpServer, powinienem to obejść w jakiś sposób? I miałbym jeszcze pytanie dotyczące wysyłania danych przez serwer, w tym przykładzie, który wstawiłem wyżej jest to w dosyć prosty sposób zrobione, a w innych przykładach które znalazłem jest tam już więcej kodu np z użyciem QByteArray itp. Da się do tego kodu niżej w jakiś banalny sposób dopisać wysyłanie używając QDataStream? Jakby to mogło wyglądać?

server.h

class Server : public QTcpServer
{
    Q_OBJECT
public:
    explicit Server(QWidget *parent = nullptr);
private slots:
    void newConnect();
    void sendMessage();
private:
    QTcpServer *tcpServer = nullptr;
    QTcpSocket *tcpClient = nullptr;
    QList<QTcpSocket>sockets;
};

server.cpp

Server::Server(QWidget *parent)
{
    tcpServer = new QTcpServer(this);
    if(!tcpServer->listen(QHostAddress::LocalHost, 3500))
        qDebug() << "Error: " << tcpServer->errorString();
    else
        qDebug() << "Server started...";
    connect(tcpServer, &QTcpServer::newConnection, this, &Server::newConnect);
}

void Server::newConnect()
{
    QTcpSocket *clientConnection = tcpServer->nextPendingConnection();
    connect(clientConnection, &QAbstractSocket::disconnected, clientConnection, &QObject::deleteLater);
    connect(tcpClient, SIGNAL(readyRead()), this, SLOT(sendMessage()));
}

void Server::sendMessage()
{

}

1 użytkowników online, w tym zalogowanych: 0, gości: 1