Problem bei Overlapped Pipe Server



  • Das hier ist mein Code. Bei waitForClient ist eine Schleife.

    1. Das erste Problem ist, ich bekomme hier Heap Corruption bei beenden. Call-Stack sieht Ihr unten.
    2. Wenn ein Client sich mit dem Server verbunden hat, Wie beende ich den Server ordentlich? Denn momentan nach beenden, bleibt es bei WaitForMultipleObjects hängen.

    // main.cpp
    PipeServer g_zpsServer;
    
    LRESULT CALLBACK MainProc( HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam ) {
    	switch ( msg ) {
    	case WM_CREATE: {
    		g_zpsServer.setPointerFillContent( fillContent );
    		g_zpsServer.setPointerSetStatusText( setStatus );
    		g_zpsServer.waitForClient();
    	} break;
    	case WM_PAINT: {
    		PAINTSTRUCT ps;
    		HDC hdc = BeginPaint( hWnd, &ps );
    
    		EndPaint( hWnd, &ps );
    	} break;
    	case WM_DESTROY:
    		PostQuitMessage( 0 );
    		break;
    	default:
    		return DefWindowProc( hWnd, msg, wParam, lParam );
    	}
    	return 0;
    }
    
    // PipeServer.cpp
    #include "stdafx.h"
    #include "PipeServer.h"
    
    TCHAR g_szNamedPipe[] = _T("\\\\.\\PIPE\\TestPipe");
    const unsigned long dwBufferSize = 1024;
    
    PipeServer::PipeServer():
    	m_pFillContent( NULL ),
    	m_pSetText( NULL ) {
        ZeroMemory( &this->m_NamedPipe, sizeof( NamedPipe ) );
        this->m_NamedPipe.hNamedPipe = INVALID_HANDLE_VALUE;
        // this->m_vEvents.push_back( CreateEvent( NULL, FALSE, FALSE, NULL ) );
    }
    
    PipeServer::~PipeServer() {
        this->disconnectAll();
    }
    
    void PipeServer::setPointerFillContent( const pfillContent pFillContent ) {
    	this->m_pFillContent = pFillContent;
    }
    
    void PipeServer::setPointerSetStatusText( const psetStatus pSetText ) {
    	this->m_pSetText = pSetText;
    }
    
    void PipeServer::waitForClient() {
        for( int i = 0; i < 3; i++ ) {
            this->m_vEvents.push_back( CreateEvent( NULL, TRUE, TRUE, NULL ) );
            this->m_vNamedPipe.push_back( this->m_NamedPipe );
    
            NamedPipe* p = &this->m_vNamedPipe[ i ];
            HANDLE hEvent = this->m_vEvents[ i ];
    
            if ( hEvent == NULL ) {
                return;
            }
    
            p->ol.hEvent = hEvent;
            p->hNamedPipe = CreateNamedPipe(
    									    g_sNamedPipe,
    									    PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED,
    									    PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
    									    PIPE_UNLIMITED_INSTANCES,
    									    dwBufferSize,
    									    dwBufferSize,
    									    NMPWAIT_USE_DEFAULT_WAIT,
    									    NULL
    									    );
            if ( p->hNamedPipe == INVALID_HANDLE_VALUE || ConnectNamedPipe( p->hNamedPipe, &p->ol ) ) {
    	        return;
            }
    
            switch( GetLastError() ) {
            case ERROR_IO_PENDING:      p->bPending = true;         break;
            case ERROR_PIPE_CONNECTED:  SetEvent( p->ol.hEvent );   break;
            default: return;
            }
    
            p->dwState = ( p->bPending )? ZP_CONNECTING: ZP_READING;
        }
        this->startThread();
    }
    
    void PipeServer::startThread() {
        HANDLE hThread = CreateThread( NULL, 0, PipeServer::ClientThread, static_cast< void* >( this ), 0, NULL );
        if ( !hThread ) {
            for( std::vector< HANDLE >::iterator it = this->m_vEvents.begin(); it < this->m_vEvents.end(); it++ ) {
                if ( *it != INVALID_HANDLE_VALUE ) {
    		        CloseHandle( *it );
                    *it = INVALID_HANDLE_VALUE;
    	        }
            }
            for( std::vector< NamedPipe >::iterator it = this->m_vNamedPipe.begin(); it < this->m_vNamedPipe.end(); it++ ) {
                if ( it->hNamedPipe != INVALID_HANDLE_VALUE ) {
    		        FlushFileBuffers( it->hNamedPipe );
    		        DisconnectNamedPipe( it->hNamedPipe );
    		        CloseHandle( it->hNamedPipe );
                    it->hNamedPipe = INVALID_HANDLE_VALUE;
    	        }
            }
        }
    }
    
    void PipeServer::disconnectAll() {
        for( std::vector< HANDLE >::iterator it = this->m_vEvents.begin(); it < this->m_vEvents.end(); it++ ) {
            if ( *it != INVALID_HANDLE_VALUE ) {
    		    CloseHandle( *it );
                *it = INVALID_HANDLE_VALUE;
    	    }
        }
        for( std::vector< NamedPipe >::iterator it = this->m_vNamedPipe.begin(); it < this->m_vNamedPipe.end(); it++ ) {
            if ( it->hNamedPipe != INVALID_HANDLE_VALUE ) {
    		    DisconnectNamedPipe( it->hNamedPipe );
    		    CloseHandle( it->hNamedPipe );
                it->hNamedPipe = INVALID_HANDLE_VALUE;
    	    }
        }
    }
    
    unsigned long __stdcall PipeServer::ClientThread( void* pParam ) {
        PipeServer* pServer = static_cast< PipeServer* >( pParam );
        unsigned long dwWait = 0, dwInstaceCount = pServer->m_vEvents.size(), dwBytesTransfered = 0, dwOldState = 0, i = 0;
        int bSuccess = FALSE;
        bool bSentSymbol = false;
        NamedPipe* p = NULL;
    
        while( true ) {
            dwWait = WaitForMultipleObjects( dwInstaceCount, &pServer->m_vEvents[ 0 ], FALSE, INFINITE );
            i = dwWait - WAIT_OBJECT_0;
            if ( i < 0 || i > dwInstaceCount - 1 ) return( -1 );
            p = &pServer->m_vNamedPipe[ i ];
            if ( p->bPending ) {
                bSuccess = GetOverlappedResult( p->hNamedPipe, &p->ol, &dwBytesTransfered, FALSE );
                switch( p->dwState ) {
                case ZP_CONNECTING:
                    if ( !bSuccess ) return( -1 );
                    p->bConnected = true;
                    p->dwState = ZP_WRITING;
                    pServer->m_pSetText( _T("Pipe client connected.") );
                    break;
                case ZP_READING:
                    if ( !bSuccess || dwBytesTransfered == 0 ) continue;
                    p->dwBytesRead = dwBytesTransfered;
                    p->dwState = ZP_WRITING;
                    break;
                case ZP_WRITING:
                    if ( !bSuccess || ( bSentSymbol && dwBytesTransfered != p->dwBytesToWrite ) ) continue;
                    p->dwState = ZP_READING;
                    bSentSymbol = true;
                    break;
                default: break;
                }
            }
    
            switch( p->dwState ) {
            case ZP_READING:
                if ( pServer->Receive( *p ) ) {
                    p->dwState = ZP_WRITING;
                }
                break;
            case ZP_WRITING:
                if ( pServer->Send( *p ) ) {
                    p->dwState = ZP_READING;
                }
                break;
            }
        }
    
    	return( 0 );
    }
    
    bool PipeServer::Receive( NamedPipe& p ) {
        int bSuccess = ReadFile(
    							p.hNamedPipe,
    							p.szInBuffer,
    							dwBufferSize * sizeof( TCHAR ),
    							&p.dwBytesRead,
                                &p.ol
    							);
        if ( bSuccess && p.dwBytesRead > 0 ) {
            p.bPending  = false;
        }
        if ( !bSuccess ) {
            unsigned long dwError = GetLastError();
            if ( dwError == ERROR_IO_PENDING ) {
                // _stprintf_s( this->m_szMsg, _T( "Pipe read still pending." ) );
                p.bPending  = true;
            } else if ( dwError == ERROR_BROKEN_PIPE ) {
                _stprintf_s( this->m_szMsg, _T( "Pipe client disconnected." ) );
                p.bConnected = false;
            } else {
                _stprintf_s( this->m_szMsg, _T("ReadFile failed, Error = %d."), dwError );
            }
            this->m_pSetText( this->m_szMsg );
            return( false );
        }
        this->parseMsg( p.szInBuffer, p.dwBytesRead / sizeof( TCHAR ) );
        this->fillContent();
    
        return( true );
    }
    
    bool PipeServer::Send( NamedPipe& p ) {
    	int bSuccess = WriteFile(
    						    p.hNamedPipe,
    						    p.szInBuffer,
                                p.dwBytesToWrite,
                                &p.dwBytesWritten,
                                &p.ol
    						    );
    
        if ( bSuccess && p.dwBytesToWrite == p.dwBytesWritten )  {
            p.bPending = false;
        }
        if ( !bSuccess && ( GetLastError() == ERROR_IO_PENDING ) ) {
            p.bPending = true;
            _stprintf_s( this->m_szMsg, _T( "Pipe write still pending." ) );
            this->m_pSetText( this->m_szMsg );
            return( false );
        }
    
        return( true );
    }
    
    void PipeServer::parseMsg( const TCHAR* szMsg, const unsigned int uLength ) {
    
    }
    
    void PipeServer::fillContent() {
    
    }
    

    Call Stack

    > msvcr90d.dll!_free_base(void * pBlock=0x00b020e8) Line 109 + 0x13 bytes C
    msvcr90d.dll!_free_dbg_nolock(void * pUserData=0x00b02108, int nBlockUse=1) Line 1426 + 0x9 bytes C++
    msvcr90d.dll!_free_dbg(void * pUserData=0x00b02108, int nBlockUse=1) Line 1258 + 0xd bytes C++
    msvcr90d.dll!operator delete(void * pUserData=0x00b02108) Line 54 + 0x10 bytes C++
    Test2.exe!std::allocator<NamedPipe>::deallocate(NamedPipe * _Ptr=0x00b02108, unsigned int __formal=3) Line 140 + 0x9 bytes C++
    Test2.exe!std::vector<NamedPipe,std::allocator<NamedPipe> >::_Tidy() Line 1134 C++
    Test2.exe!std::vector<NamedPipe,std::allocator<NamedPipe> >::~vector<NamedPipe,std::allocator<NamedPipe> >() Line 560 C++
    Test2.exe!PipeServer::~PipeServer() Line 20 + 0x2d bytes C++
    Test2.exe!`dynamic atexit destructor for 'g_zpsServer''() + 0x28 bytes C++
    msvcr90d.dll!doexit(int code=0, int quick=0, int retcaller=0) Line 591 C
    msvcr90d.dll!exit(int code=0) Line 412 + 0xd bytes C
    Test2.exe!__tmainCRTStartup() Line 599 C
    Test2.exe!wWinMainCRTStartup() Line 403 C



  • Ich habe vergesse zu erwähnen, dass wenn die Schleife bis zu 2 mal durchläuft, gibt es kein Heap Corruption Fehler. Kann mir jemand bitte das Problem aufklären? Danke sehr.

    for( int i = 0; i < 2; i++ )
    


  • Deine OVERLAPPED Struktur steckt in einem std::vector. Klar dass du damit Probleme bekommst.



  • hustbaer schrieb:

    Deine OVERLAPPED Struktur steckt in einem std::vector. Klar dass du damit Probleme bekommst.

    Kannst Du mir bitte auch noch sagen, warum das ein Problem ist? Wie soll ich besser machen, hast Du auch noch Lösungsvorschlag?



  • WeiterhinProblematisch schrieb:

    Kannst Du mir bitte auch noch sagen, warum das ein Problem ist?

    Mit ConnectNamedPipe startest du nen asynchronen IO dem du die Adresse der OVERLAPPED Struktur in deinem vector übergiebst. Bis dieser IO abgeschlossen ist muss die OVERLAPPED Struktur an dieser Adresse bleiben.

    Danach steckst du allerdings wietere Elemente in deinen std::vector rein. Wenn der interne Puffer des std::vector zu klein wird fordert vector einen neuen an, kopiert alle Elemente in den neuen Puffer und gibt den alten dann frei. An der Stelle wird also auch die OVERLAPPED Struktur ungültig, obwohl der IO noch nicht abgeschlossen ist. Wenn der IO dann abgeschlossen oder abgebrochen wird greift Windows auf den bereits freigegebenen Speicher zu und schreibt da hinein. Was dann normalerweise zu einer "heap corruption" führt. D.h. dein Programm stürzt bei einer der nächsten Heap-Operationen ab.

    WeiterhinProblematisch schrieb:

    Wie soll ich besser machen, hast Du auch noch Lösungsvorschlag?

    Ein "quick fix" wäre in PipeServer::waitForClient , vor der for-Schleife

    this->m_vNamedPipe.reserve(3);
    

    einzufügen. Damit zwingst du den vector an dieser Stelle, also bevor der 1. asynchrone IO gestartet wird Speicher für 3 Elemente anzufordern.
    Vorausgesetzt der vector wird nie über die initialen 3 Elemente hinaus vergrössert sollte das ausreichend sein.

    Die bessere Lösung wäre vermutlich statt eines std::vector<NamedPipe> einen std::vector<std::unique_ptr<NamedPipe>> zu verwenden.

    Wenn du solche Fehler in Zukunft vermeiden willst, solltest du alle Klassen die sowas wie OVERLAPPED Strukturen enthalten noncopyable machen.

    Also

    struct NamedPipe {
        NamedPipe(NamedPipe const&) = delete;
        NamedPipe& operator=(NamedPipe const&) = delete;
    
       //...
    };
    

    oder alternativ einfach von boost::noncopyable ableiten.

    ----

    Davon abgesehen solltest du lernen wie man korrekt mit den Werkzeugen umgeht die du da verwendest. Also wie die diversen STL Container funktionieren, was sie garantieren und was nicht, wie OVERLAPPED IO funktioniert etc. Sonst wirst du relativ oft solche Fehler machen.


  • |  Mod

    Ich würde eine PipeServer Klasse bauen, die jeweils nur einen CreateNamedPipe/ConnectNamedPipe ausführt.

    Dein Konstrukt mit den Arrays is in meinen Augen auch viel zu kompliziert. Fasse die einzelnen Dinge, die zum Server, oder einer Client Verbindung passen in einer Klasse zusammen. Die halten jeder auch brav ein Handle, und entsorgend das auch wieder.

    Zudem skaliert das auch einfacher und dynamisch.

    Dann hast Du auch nicht so eine komische Schleife die alles macht (auf Connect warten und auf andere IOs.) Ich empfinde Deinen Code viel zu kompliziert... just my 2 cents.

    Server
         While not Ending
             CreateNamedPipe, ConnectNamedPipe
             Wait For Connect
                If Connect
                   Start Client Thread
                   break;
                End If
             End Wait
         End While
    
    Client Thread
         While Not Ending
             Wait For Read
             If Read
                 Process Read Data
             End If
         End While
    


  • Martin Richter schrieb:

    Ich würde eine PipeServer Klasse bauen, die jeweils nur einen CreateNamedPipe/ConnectNamedPipe ausführt.

    Dein Konstrukt mit den Arrays is in meinen Augen auch viel zu kompliziert. Fasse die einzelnen Dinge, die zum Server, oder einer Client Verbindung passen in einer Klasse zusammen. Die halten jeder auch brav ein Handle, und entsorgend das auch wieder.

    Zudem skaliert das auch einfacher und dynamisch.

    Dann hast Du auch nicht so eine komische Schleife die alles macht (auf Connect warten und auf andere IOs.) Ich empfinde Deinen Code viel zu kompliziert... just my 2 cents.

    Server
    While not Ending
    CreateNamedPipe, ConnectNamedPipe
    Wait For Connect
    If Connect
    Start Client Thread
    break;
    End If
    End Wait
    End While

    Client Thread
    While Not Ending
    Wait For Read
    If Read
    Process Read Data
    End If
    End While

    herzlichen Dank für die Auflärung, habe wieder was neues gelernt.

    Martin Richter schrieb:

    Ich würde eine PipeServer Klasse bauen, die jeweils nur einen CreateNamedPipe/ConnectNamedPipe ausführt.

    Dein Konstrukt mit den Arrays is in meinen Augen auch viel zu kompliziert. Fasse die einzelnen Dinge, die zum Server, oder einer Client Verbindung passen in einer Klasse zusammen. Die halten jeder auch brav ein Handle, und entsorgend das auch wieder.

    Zudem skaliert das auch einfacher und dynamisch.

    Dann hast Du auch nicht so eine komische Schleife die alles macht (auf Connect warten und auf andere IOs.) Ich empfinde Deinen Code viel zu kompliziert... just my 2 cents.

    Server
    While not Ending
    CreateNamedPipe, ConnectNamedPipe
    Wait For Connect
    If Connect
    Start Client Thread
    break;
    End If
    End Wait
    End While

    Client Thread
    While Not Ending
    Wait For Read
    If Read
    Process Read Data
    End If
    End While

    Auch Dir herzlichen Dank. Ich wollte erstmal so grob mit PipeServer schreiben, erst wenn alles so gut läuft, werde ich mehr oder weniger aufräumen, momentan geht es da mehr um Verständnis und schnell eine Lösung zu haben.


  • |  Mod

    Hab nochmal code tags eingeführt, dann sieht man die Struktur, die ich meine, besser.

    Das Problem ist einfach dass So ein Code-Wust im ersten Moment auch kaum durchschaubar ist. Die Segmente werden kleiner und besser testbar, wenn man es einzeln kappselt.

    Solche switch/case Strukturen, die eine Statusengine aufbauen sind meines Erachtens nicht so leicht zu verstehen.



  • Martin Richter schrieb:

    Hab nochmal code tags eingeführt, dann sieht man die Struktur, die ich meine, besser.

    Das Problem ist einfach dass So ein Code-Wust im ersten Moment auch kaum durchschaubar ist. Die Segmente werden kleiner und besser testbar, wenn man es einzeln kappselt.

    Solche switch/case Strukturen, die eine Statusengine aufbauen sind meines Erachtens nicht so leicht zu verstehen.

    Klar, verstehe ich. Ich habe den Code von msdn übernommen, habe es nur leicht verändert.

    https://msdn.microsoft.com/en-us/library/windows/desktop/aa365603(v=vs.85).aspx