Code review comment for lp://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication

Revision history for this message
Mike McCracken (mikemc) wrote :

The code is a little fragile wrt crashes. (This branch hasn't made it any worse, but now's a good time to fix this, since it's short and related.)

If CP crashes and doesn't get to call removeServer, then the socket file is left around and future servers can't be started with the same name, so from then on, every invocation of CP will fail to connect to a server, think it's the unique instance, and start up happily.

I'd like to suggest a couple of improvements:
1. check self.ready, the response from server.listen. If this is false, log server.errorString(). In the current code, we assume server.listen always works and having this log would've made it a lot easier to find the problem I was seeing.

2. call self.cleanup() (or just server.removeServer) before server.listen. In the event that another CP has crashed, leaving around a socket file, this will clean it up so server.listen should always succeed like we want.

This should be ok to call unconditionally, because if we get to this line, we've failed to connect to any server on that socket, and we really should be the unique instance.

NOTE: windows doesn't use socket files, but Qt says that removeServer does nothing on windows, so at least this shouldn't break anything.

With these changes, I am seeing no problems on osx even when I cause a CP crash and verify that there's a zombie socket file before I try re-starting CP.

review: Needs Fixing

« Back to merge proposal