InterprocessConnection::disconnect() crashes app


#1

Calling disconnect() in release mode in Windows from the side that created the pipe after the other side of the pipe crashed is crashing the 2nd side too.

The line

inside disconnect() is the one that crashes, probably because

pipe  ->read (messageHeader, sizeof (messageHeader), pipeReceiveMessageTimeout);

is blocking.

Is there anything to do about that?
Why this is only happening in release mode?

Thanks


#2

Ok, I think I found the problem (and the solution):

After the call pimpl->disconnectPipe() inside NamedPipe::read() (excerption below)
calling isConnected() still returns true because isConnected() is actually the check pimpl != nullptr and pimpl still exists.

So all I did was to delete it after disconnectPipe():

                else if ((GetLastError() == ERROR_BROKEN_PIPE || GetLastError() == ERROR_PIPE_NOT_CONNECTED) && pimpl->isPipe)
                {
                    pimpl->disconnectPipe();
		    pimpl = nullptr;           //   <- deleting pimpl
                    waitAgain = true;
                }

This way isConnected() returns false, and everything is alright now.

Edit: There’s another call to pimpl->disconnectPipe() in read() so this call should too be followed by pimpl = nullptr;


#3

Ok, thanks! I’ll take a look at that…


#4

Hi Jules

Your fix now causes isOpen() to return false instead of true in InterprocessConnection::run():

        else if (pipe != nullptr)
        {
            if (! pipe->isOpen())
            {
                {
                    const ScopedLock sl (pipeAndSocketLock);
                    pipe = nullptr;
                }

                connectionLostInt();
                break;
            }

Which causes a call to connectionLost() upon starting a pipe.


#5

Ok, but that would also have happened in your code, wouldn’t it? If you delete the pimpl, then isOpen() would also have returned false. (?)

Your original change seemed to break all the logic in that function - if it deletes the pimpl, then there’s no point in setting the waitAgain flag because the loop will exit immediately and won’t attempt to re-open the broken pipe. That’s not how the function was intended to work - if there’s an error, the pimpl object should be kept, but allowed to try to re-connect.


#6

You are right, for my purposes it’s fine cause I’m creating a new InterprocesssConnection object after each broken connection.
Anyway, you can’t leave it like that cause it just deletes the pipe right after starting it:

        else if (pipe != nullptr)
        {
            if (! pipe->isOpen())
            {
                {
                    const ScopedLock sl (pipeAndSocketLock);
                    pipe = nullptr;              //<- deletes the pipe
                }

                connectionLostInt();
                break;
            }

It’s hard for me to understand all the logic in there, but I think there’s not a clean distinction between an open pipe which means a server pipe
that created a pipe or a client pipe that has opened a server pipe file and a connected pipe which in a client pipe is the same but for a server pipe it means that there’s a client pipe on the other side.

This line is even more complicating things for me:

            if (ConnectNamedPipe (pipeH, &over))
            {
                connected = false;  // yes, you read that right. In overlapped mode it should always return 0.
            }

And also since the triggering of the connectionMade/Lost is different in Windows/Mac I’ve just created my own isConnected() method which is based on constantly sending acks between the server/client pipes.

One last minor thing, in InterprocessConnection::disconnect() there is no need for the line pipe->cancelPendingReads() since it’s the first line inside pipe->close()

    if (pipe != nullptr)
    {
        pipe->cancelPendingReads();
        pipe->close();
    }

Shlomi


#7

Gah… My brain hurts. I really hate those classes - they’re really old, some of the content was contributed by other people, and since I don’t really use them myself, I’ve not got a 100% grasp on the logic in there quite as much as most of the codebase. I’m also afraid of changing anything in case it breaks user code. Would like to give them a serious review, but am a bit too stretched right now.


#8

Using the TIP it seems NamedPipe’s are broken when you haven’t started a server before making a connection.

To reproduce, use the demo and chose Named Pipe (connect to existing pipe) [yes none exists]

[EDIT] - Now Quit the app.

Deadlocked on these threads… I think previously(sorry, its been a while since I pulled the tip), if the named pipe didn’t exist when trying to connect it wouldn’t start a read thread. Right now the Pimpl is always created

#0  0x929a1aa2 in __semwait_signal ()
#1  0x929a175e in _pthread_cond_wait ()
#2  0x929a12b1 in pthread_cond_timedwait$UNIX2003 ()
#3  0x0018f72f in juce::WaitableEvent::wait (this=0xf5edac, timeOutMillisecs=100) at juce_posix_SharedCode.h:117
#4  0x0018fb3f in juce::ReadWriteLock::enterWrite (this=0xf5eda8) at juce_ReadWriteLock.cpp:119
#5  0x001d3d1f in juce::ScopedWriteLock::ScopedWriteLock (this=0xbfffe904, lock=@0xf5eda8) at juce_ScopedWriteLock.h:69
#6  0x00196225 in juce::NamedPipe::close (this=0xf5eda0) at juce_posix_NamedPipe.cpp:183
#7  0x0021cef5 in juce::InterprocessConnection::disconnect (this=0xf5ec70) at juce_InterprocessConnection.cpp:107
#8  0x0021d31d in juce::InterprocessConnection::~InterprocessConnection (this=0xf5ec70) at juce_InterprocessConnection.cpp:39
#9  0x0002512e in InterprocessCommsDemo::DemoInterprocessConnection::~DemoInterprocessConnection (this=0xf5ec70) at /Volumes/Reznor/jucegitmodules/extras/JuceDemo/Builds/MacOSX/../../Source/demos/InterprocessCommsDemo.cpp:243
#0  0x929a1aa2 in __semwait_signal ()
#1  0x929cd9c5 in nanosleep$UNIX2003 ()
#2  0x0018e77c in juce::Thread::sleep (millisecs=2) at juce_posix_SharedCode.h:155
#3  0x001d30a5 in juce::NamedPipe::Pimpl::openPipe (name=@0xf5ee24, flags=6, timeoutEnd=0) at juce_posix_NamedPipe.cpp:153
#4  0x001d31fd in juce::NamedPipe::Pimpl::read (this=0xf5ee20, destBuffer=0xb0102e80 "\001", maxBytesToRead=8, timeOutMilliseconds=-1) at juce_posix_NamedPipe.cpp:58
#5  0x00191455 in juce::NamedPipe::read (this=0xf5eda0, destBuffer=0xb0102e80, maxBytesToRead=8, timeOutMilliseconds=-1) at juce_posix_NamedPipe.cpp:209
#6  0x0021c574 in juce::InterprocessConnection::readNextMessageInt (this=0xf5ec70) at juce_InterprocessConnection.cpp:266
#7  0x0021c9a8 in juce::InterprocessConnection::run (this=0xf5ec70) at juce_InterprocessConnection.cpp:357
#8  0x0018f987 in juce::Thread::threadEntryPoint (this=0xf5ec70) at juce_Thread.cpp:93
#9  0x0018f9f8 in juce::juce_threadEntryPoint (userData=0xf5ec70) at juce_Thread.cpp:105
#10 0x0018fa17 in threadEntryProc (userData=0xf5ec70) at juce_posix_SharedCode.h:838

#9

Thanks! Should be fixed now.