Problem bei Overlapped Pipe Server

[WeiterhinProblematisch]

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

[WeiterhinProblematisch]

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++ )

[hustbaer]

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

[WeiterhinProblematisch]

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?

[hustbaer]

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.

[Martin Richter]

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

[WeiterhinProblematisch]

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.

[Martin Richter]

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.

[WeiterhinProblematisch]

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