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

Revision history for this message
Diego Sarmentero (diegosarmentero) 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.

Done!

« Back to merge proposal