Hi,
Today I had the chance the experience another explorer lookup and was
able to attach to the explorer process using VS2003, having the complete
TSVN debug info present. I found out that the explorer hang at the
WaitForSingleObject(m_hEvent) command waiting for the pipe transacation
to complete. The event was never signaled.
I then reviewed the code around the pipe opearions and this is what I
found out:
#1: The event created for the overlapped opeartion was created as a
named sync object. That means that multiple instances of explorer.exe
would share the SAME event object. So when two instances of the explorer
are talking to the cache simultaneously, one finished transaction might
wake up both waiting clients, leaving one with wrong data.
#2: The event object was created with bManualReset=FALSE, creating an
auto-reset event. According to the documentation of overlapped I/O and
TransactNamedPipe, you should better pass a manual-reset event. However,
I do not think this causes a problem on it's own, but it can definately
lead to deadlocks in combination with issue #1.
#3: The case where TransactNamedPipe returns TRUE was not handled
correctly. Even if it is not very likely, overlapped opeartions *can* to
my knowledge complete immediatley, perhaps(?) leaving the overlapped
structure and its event object untouched (I did not find a proove of
this in the docs, so we should be prepared for either behaviour).
WaitForSingleObject was still called in this case, possibly waiting
forever for a transaction that might already be finished.
#4: The pipe server in TSVNCache calls WaitNamePipe(). Though I do not
believe it causes any harm there, it is normally used on the client side
to wait for the next free instance of a pipe before connecting to the
pipe. It should be called if the OpenFile errors out with a
ERROR_PIPE_BUSY error.
I created a patch that rearranged the pipe calls to clean up these
issues. I am just rebuilding all, so you should not commit it until
tested. In the meantime please feel free to comment.
Norbert
Index: src/TortoiseShell/RemoteCacheLink.h
===================================================================
--- src/TortoiseShell/RemoteCacheLink.h (revision 3183)
+++ src/TortoiseShell/RemoteCacheLink.h (working copy)
@@ -14,6 +14,7 @@
private:
bool EnsurePipeOpen();
+ void ClosePipe();
private:
HANDLE m_hPipe;
Index: src/TortoiseShell/RemoteCacheLink.cpp
===================================================================
--- src/TortoiseShell/RemoteCacheLink.cpp (revision 3183)
+++ src/TortoiseShell/RemoteCacheLink.cpp (working copy)
@@ -31,49 +31,79 @@
return true;
}
- m_hPipe = CreateFile(
- _T("\\\\.\\pipe\\TSVNCache"), // pipe name
- GENERIC_READ | // read and write access
- GENERIC_WRITE,
- 0, // no sharing
+ m_hPipe = CreateFile(
+ _T("\\\\.\\pipe\\TSVNCache"), // pipe name
+ GENERIC_READ | // read and write access
+ GENERIC_WRITE,
+ 0, // no sharing
NULL, // default security attributes
- OPEN_EXISTING, // opens existing pipe
- FILE_FLAG_OVERLAPPED, // default attributes
- NULL); // no template file
+ OPEN_EXISTING, // opens existing pipe
+ FILE_FLAG_OVERLAPPED, // default attributes
+ NULL); // no template file
+ if (m_hPipe == INVALID_HANDLE_VALUE && GetLastError() == ERROR_PIPE_BUSY)
+ {
+ // TSVNCache is running but is busy connecting a different client.
+ // Do not give up immediately but wait for a few milliseconds until
+ // the server has created the next pipe instance
+ if (WaitNamedPipe(_T("\\\\.\\pipe\\TSVNCache"), 50))
+ {
+ m_hPipe = CreateFile(
+ _T("\\\\.\\pipe\\TSVNCache"), // pipe name
+ GENERIC_READ | // read and write access
+ GENERIC_WRITE,
+ 0, // no sharing
+ NULL, // default security attributes
+ OPEN_EXISTING, // opens existing pipe
+ FILE_FLAG_OVERLAPPED, // default attributes
+ NULL); // no template file
+ }
+ }
- if (m_hPipe != INVALID_HANDLE_VALUE)
+
+ if (m_hPipe != INVALID_HANDLE_VALUE)
{
- // The pipe connected; change to message-read mode.
- DWORD dwMode;
+ // The pipe connected; change to message-read mode.
+ DWORD dwMode;
- dwMode = PIPE_READMODE_MESSAGE;
- if(!SetNamedPipeHandleState(
- m_hPipe, // pipe handle
- &dwMode, // new pipe mode
- NULL, // don't set maximum bytes
- NULL)) // don't set maximum time
+ dwMode = PIPE_READMODE_MESSAGE;
+ if(!SetNamedPipeHandleState(
+ m_hPipe, // pipe handle
+ &dwMode, // new pipe mode
+ NULL, // don't set maximum bytes
+ NULL)) // don't set maximum time
{
- ATLTRACE("SetNamedPipeHandleState failed");
+ ATLTRACE("SetNamedPipeHandleState failed");
CloseHandle(m_hPipe);
m_hPipe = INVALID_HANDLE_VALUE;
return false;
}
- m_hEvent = CreateEvent(NULL, FALSE, FALSE, _T("TSVNCache_OverlappedClientWait"));
+ // create an unnamed (=local) manual reset event for use in the overlapped structure
+ m_hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
if (m_hEvent)
return true;
- ATLTRACE("CreateEvent failed");
- CloseHandle(m_hPipe);
- m_hPipe = INVALID_HANDLE_VALUE;
- CloseHandle(m_hEvent);
- m_hEvent = INVALID_HANDLE_VALUE;
+ ATLTRACE("CreateEvent failed");
+ ClosePipe();
return false;
-
}
return false;
}
+
+void CRemoteCacheLink::ClosePipe()
+{
+ AutoLocker lock(m_critSec);
+
+ if(m_hPipe == INVALID_HANDLE_VALUE)
+ {
+ CloseHandle(m_hPipe);
+ CloseHandle(m_hEvent);
+ m_hPipe = INVALID_HANDLE_VALUE;
+ m_hEvent = INVALID_HANDLE_VALUE;
+ }
+}
+
bool CRemoteCacheLink::GetStatusFromRemoteCache(const CTSVNPath& Path, TSVNCacheResponse* pReturnedStatus, bool bRecursive)
{
if(!EnsurePipeOpen())
@@ -100,7 +130,7 @@
sCachePath.ReleaseBuffer();
ATLTRACE("Failed to start cache\n");
return false;
- }
+ }
sCachePath.ReleaseBuffer();
// Wait for the cache to open
@@ -117,7 +147,7 @@
AutoLocker lock(m_critSec);
- DWORD nBytesRead;
+ DWORD nBytesRead;
TSVNCacheRequest request;
request.flags = TSVNCACHE_FLAGS_NONOTIFICATIONS;
if(bRecursive)
@@ -133,47 +163,53 @@
// A blocked shell is a very bad user impression, because users
// who don't know why it's blocked might find the only solution
// to such a problem is a reboot and therefore they might loose
- // valuable data.
+ // valuable data.
// Sure, it would be better to have no situations where the shell
// even can get blocked, but the timeout of 5 seconds is long enough
// so that users still recognize that something might be wrong and
// report back to us so we can investigate further.
- if (!TransactNamedPipe(m_hPipe, &request, sizeof(request), pReturnedStatus, sizeof(*pReturnedStatus), &nBytesRead, &m_Overlapped))
+
+ BOOL fSuccess = TransactNamedPipe(m_hPipe,
+ &request, sizeof(request),
+ pReturnedStatus, sizeof(*pReturnedStatus),
+ &nBytesRead, &m_Overlapped);
+
+ if (!fSuccess)
{
if (GetLastError()!=ERROR_IO_PENDING)
{
- OutputDebugStringA("TortoiseShell: TransactNamedPipe failed\n");
- CloseHandle(m_hPipe);
- CloseHandle(m_hEvent);
- m_hPipe = INVALID_HANDLE_VALUE;
- m_hEvent = INVALID_HANDLE_VALUE;
+ OutputDebugStringA("TortoiseShell: TransactNamedPipe failed\n");
+ ClosePipe();
return false;
}
+
+ // TransactNamedPipe is working in an overlapped operation.
+ // Wait for it to finish
+ DWORD dwWait = WaitForSingleObject(m_hEvent, INFINITE);
+ if (dwWait == WAIT_OBJECT_0)
+ {
+ fSuccess = GetOverlappedResult(m_hPipe, &m_Overlapped, &nBytesRead, FALSE);
+ }
+ else
+ fSuccess = FALSE;
}
- DWORD dwWait = WaitForSingleObject(m_hEvent, INFINITE);
- if (dwWait == WAIT_OBJECT_0)
+ if (fSuccess)
{
- if (GetOverlappedResult(m_hPipe, &m_Overlapped, &nBytesRead, FALSE))
+ if(nBytesRead == sizeof(TSVNCacheResponse))
{
- if(nBytesRead == sizeof(TSVNCacheResponse))
- {
- // This is a full response - we need to fix-up some pointers
- pReturnedStatus->m_status.entry = &pReturnedStatus->m_entry;
- pReturnedStatus->m_entry.url = pReturnedStatus->m_url;
- }
- else
- {
- pReturnedStatus->m_status.entry = NULL;
- }
+ // This is a full response - we need to fix-up some pointers
+ pReturnedStatus->m_status.entry = &pReturnedStatus->m_entry;
+ pReturnedStatus->m_entry.url = pReturnedStatus->m_url;
+ }
+ else
+ {
+ pReturnedStatus->m_status.entry = NULL;
+ }
- return true;
- }
+ return true;
}
- OutputDebugStringA("TortoiseShell: WaitForSingleObject failed - Pipe timeout\n");
- CloseHandle(m_hPipe);
- CloseHandle(m_hEvent);
- m_hPipe = INVALID_HANDLE_VALUE;
- m_hEvent = INVALID_HANDLE_VALUE;
+ OutputDebugStringA("TortoiseShell: WaitForSingleObject failed - Pipe timeout\n");
+ ClosePipe();
return false;
}
Index: src/TSVNCache/TSVNCache.cpp
===================================================================
--- src/TSVNCache/TSVNCache.cpp (revision 3183)
+++ src/TSVNCache/TSVNCache.cpp (working copy)
@@ -369,50 +369,43 @@
continue; // never leave the thread!
}
SetSecurityInfo(hPipe, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION, 0, 0, 0, 0);
- if (WaitNamedPipe(TSVN_CACHE_PIPE_NAME, 1000))
- {
- // Wait for the client to connect; if it succeeds,
- // the function returns a nonzero value. If the function returns
- // zero, GetLastError returns ERROR_PIPE_CONNECTED.
- fConnected = ConnectNamedPipe(hPipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
- if (fConnected)
- {
- // Create a thread for this client.
- hInstanceThread = CreateThread(
- NULL, // no security attribute
- 0, // default stack size
- (LPTHREAD_START_ROUTINE) InstanceThread,
- (LPVOID) hPipe, // thread parameter
- 0, // not suspended
- &dwThreadId); // returns thread ID
- if (hInstanceThread == NULL)
- {
- OutputDebugStringA("TSVNCache: Could not create Instance thread\n");
- DebugOutputLastError();
- DisconnectNamedPipe(hPipe);
- CloseHandle(hPipe);
- // since we're now closing this thread, we also have to close the whole application!
- // otherwise the thread is dead, but the app is still running, refusing new instances
- // but no pipe will be available anymore.
- PostMessage(hWnd, WM_CLOSE, 0, 0);
- return;
- }
- else CloseHandle(hInstanceThread);
- }
- else
+ // Wait for the client to connect; if it succeeds,
+ // the function returns a nonzero value. If the function returns
+ // zero, GetLastError returns ERROR_PIPE_CONNECTED.
+ fConnected = ConnectNamedPipe(hPipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
+ if (fConnected)
+ {
+ // Create a thread for this client.
+ hInstanceThread = CreateThread(
+ NULL, // no security attribute
+ 0, // default stack size
+ (LPTHREAD_START_ROUTINE) InstanceThread,
+ (LPVOID) hPipe, // thread parameter
+ 0, // not suspended
+ &dwThreadId); // returns thread ID
+
+ if (hInstanceThread == NULL)
{
- // The client could not connect, so close the pipe.
- OutputDebugStringA("TSVNCache: ConnectNamedPipe failed\n");
+ OutputDebugStringA("TSVNCache: Could not create Instance thread\n");
DebugOutputLastError();
- CloseHandle(hPipe);
- continue; // don't end the thread!
+ DisconnectNamedPipe(hPipe);
+ CloseHandle(hPipe);
+ // since we're now closing this thread, we also have to close the whole application!
+ // otherwise the thread is dead, but the app is still running, refusing new instances
+ // but no pipe will be available anymore.
+ PostMessage(hWnd, WM_CLOSE, 0, 0);
+ return;
}
- }
+ else CloseHandle(hInstanceThread);
+ }
else
{
- CloseHandle(hPipe);
- continue; // don't end the thread!
+ // The client could not connect, so close the pipe.
+ OutputDebugStringA("TSVNCache: ConnectNamedPipe failed\n");
+ DebugOutputLastError();
+ CloseHandle(hPipe);
+ continue; // don't end the thread!
}
}
ATLTRACE("Pipe thread exited\n");
@@ -452,50 +445,43 @@
continue; // never leave the thread!
}
SetSecurityInfo(hPipe, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION, 0, 0, 0, 0);
- if (WaitNamedPipe(TSVN_CACHE_COMMANDPIPE_NAME, 1000))
- {
- // Wait for the client to connect; if it succeeds,
- // the function returns a nonzero value. If the function returns
- // zero, GetLastError returns ERROR_PIPE_CONNECTED.
- fConnected = ConnectNamedPipe(hPipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
- if (fConnected)
- {
- // Create a thread for this client.
- hCommandThread = CreateThread(
- NULL, // no security attribute
- 0, // default stack size
- (LPTHREAD_START_ROUTINE) CommandThread,
- (LPVOID) hPipe, // thread parameter
- 0, // not suspended
- &dwThreadId); // returns thread ID
- if (hCommandThread == NULL)
- {
- OutputDebugStringA("TSVNCache: Could not create Command thread\n");
- DebugOutputLastError();
- DisconnectNamedPipe(hPipe);
- CloseHandle(hPipe);
- // since we're now closing this thread, we also have to close the whole application!
- // otherwise the thread is dead, but the app is still running, refusing new instances
- // but no pipe will be available anymore.
- PostMessage(hWnd, WM_CLOSE, 0, 0);
- return;
- }
- else CloseHandle(hCommandThread);
- }
- else
+ // Wait for the client to connect; if it succeeds,
+ // the function returns a nonzero value. If the function returns
+ // zero, GetLastError returns ERROR_PIPE_CONNECTED.
+ fConnected = ConnectNamedPipe(hPipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
+ if (fConnected)
+ {
+ // Create a thread for this client.
+ hCommandThread = CreateThread(
+ NULL, // no security attribute
+ 0, // default stack size
+ (LPTHREAD_START_ROUTINE) CommandThread,
+ (LPVOID) hPipe, // thread parameter
+ 0, // not suspended
+ &dwThreadId); // returns thread ID
+
+ if (hCommandThread == NULL)
{
- // The client could not connect, so close the pipe.
- OutputDebugStringA("TSVNCache: ConnectNamedPipe failed\n");
+ OutputDebugStringA("TSVNCache: Could not create Command thread\n");
DebugOutputLastError();
- CloseHandle(hPipe);
- continue; // don't end the thread!
+ DisconnectNamedPipe(hPipe);
+ CloseHandle(hPipe);
+ // since we're now closing this thread, we also have to close the whole application!
+ // otherwise the thread is dead, but the app is still running, refusing new instances
+ // but no pipe will be available anymore.
+ PostMessage(hWnd, WM_CLOSE, 0, 0);
+ return;
}
- }
+ else CloseHandle(hCommandThread);
+ }
else
{
- CloseHandle(hPipe);
- continue; // don't end the thread!
+ // The client could not connect, so close the pipe.
+ OutputDebugStringA("TSVNCache: ConnectNamedPipe failed\n");
+ DebugOutputLastError();
+ CloseHandle(hPipe);
+ continue; // don't end the thread!
}
}
ATLTRACE("CommandWait thread exited\n");
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tortoisesvn.tigris.org
For additional commands, e-mail: dev-help@tortoisesvn.tigris.org
Received on Sat Apr 30 11:35:12 2005