Remove NamedPipeInternal::connected?


#1

As I stare longer and longer at the Windows NamedPipeInternal code, I’m concerned about NamedPipeInternal::connected and having it get stepped on by multiple threads. What if one thread calls disconnectPipe and in between the return of DisconnectPipe and the assignment of connected to false, another thread calls connect. It checks the connected member variable and it’s true, so it thinks the pipe is connected and returns true when that’s not really true. I have a feeling there are other racy scenarios.

I suppose some function down the line will fail as a result so it’s not the end of the world, but perhaps there’s a simpler way? What if we remove the NamedPipeInternal::connected member variable altogether? Each time NamedPipeInternal::disconnectPipe gets called, DisconnectNamedPipe gets called. And each time NamedPipeInternal::connect() gets called, ConnectNamedPipe gets called. Only for the master side of the pipe that is. We leave it to the OS to manage the state of the pipe safely.

I’ll try this and see what happens. I’ll also see if I can come up with an actual test to demonstrate something bad with the code that’s there now though nothing comes to mind at the moment.


#2

I don’t think it’d be very wise to have multiple threads trying to use the same NamedPipe simultaneously, regardless of what the connected() method does. The class was never intended to be thread-safe, so you should either use your own lock to restrict access to it, or just make sure only one thread uses it.


#3

With an InterprocessConnection it seems pretty likely that multiple threads use the same NamedPipe simultaneously. The read thread is there and then anyone can call sendMessage any time. There is a CriticalSection in InterprocessConnection but it doesn’t seem to prevent anything like this. I’m not sure it should either since reading from and writing to a pipe seem like natural things to do in parallel.