[Freeciv-Dev] (PR#12346) incorrect saving when quitting too fast

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=12346 >

At first I thought it should be fixed with signals.  Send a SIGTERM to
the server rather than a SIGKILL, and interpret a SIGTERM properly in
the server (block it during saves, auto-save otherwise).  But this isn't
really portable since windows uses CloseProcess whose behavior I don't
know; signals on other platforms may not have consistent behavior.

The other alternative is to use /quit to kill the server.  Whenever the
client chooses to disconnect, do a "soft" kill of the server by sending
/quit instead of a signal to kill it.  This should cause the server to
quit in its own good time even if this means running a few extra seconds
to complete a save.  The problem here is sometimes the client doesn't
choose to disconnect and we have to do a full kill.

The attached patch is an attempt at the second.  This needs some testing
but it should probably go into 2.0 and the dev version.

-jason

Index: client/clinet.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/clinet.c,v
retrieving revision 1.117
diff -p -u -r1.117 clinet.c
--- client/clinet.c     1 Aug 2005 06:48:28 -0000       1.117
+++ client/clinet.c     8 Aug 2005 19:25:51 -0000
@@ -122,7 +122,7 @@ static void close_socket_callback(struct
 {
   close_socket_nomessage(pc);
   /* If we lost connection to the internal server - kill him */
-  client_kill_server();
+  client_kill_server(TRUE);
   append_output_window(_("Lost connection to server!"));
   freelog(LOG_NORMAL, "lost connection to server");
 }
@@ -240,10 +240,16 @@ int try_to_connect(const char *username,
 **************************************************************************/
 void disconnect_from_server(void)
 {
-  close_socket_nomessage(&aconnection);
+  bool force = !aconnection.used;
+
   /* If it's internal server - kill him 
    * We assume that we are always connected to the internal server  */
-  client_kill_server();
+  client_kill_server(force);
+
+  close_socket_nomessage(&aconnection);
+  if (force) {
+    client_kill_server(TRUE);
+  }
   append_output_window(_("Disconnected from server."));
 }  
 
Index: client/connectdlg_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/connectdlg_common.c,v
retrieving revision 1.31
diff -p -u -r1.31 connectdlg_common.c
--- client/connectdlg_common.c  11 Mar 2005 17:11:25 -0000      1.31
+++ client/connectdlg_common.c  8 Aug 2005 19:25:51 -0000
@@ -127,25 +127,53 @@ bool can_client_access_hack(void)
   return client_has_hack;
 }
 
-/************************************************************************** 
-Kills the server if the client has started it.
-**************************************************************************/ 
-void client_kill_server()
+/****************************************************************************
+  Kills the server if the client has started it.
+
+  If the 'force' parameter is unset, we just do a /quit.  If it's set, then
+  we'll send a signal to the server to kill it (use this when the socket
+  is disconnected already).
+****************************************************************************/
+void client_kill_server(bool force)
 {
   if (is_server_running()) {
+    if (aconnection.used) {
+      /* This does a "soft" shutdown of the server by sending a /quit.
+       *
+       * This is useful when closing the client or disconnecting because it
+       * doesn't kill the server prematurely.  In particular, killing the
+       * server in the middle of a save can have disasterous results.  This
+       * method tells the server to quit on its own.  This is safer from a
+       * game perspective, but more dangerous because if the kill fails the
+       * server will be left running.
+       *
+       * Another potential problem is because this function is called atexit
+       * it could potentially be called when we're connected to an unowned
+       * server.  In this case we don't want to kill it. */
+      send_chat("/quit");
 #ifdef WIN32_NATIVE
-    TerminateProcess(server_process, 0);
-    CloseHandle(server_process);
-    if (loghandle != INVALID_HANDLE_VALUE) {
-      CloseHandle(loghandle);
-    }
-    server_process = INVALID_HANDLE_VALUE;
-    loghandle = INVALID_HANDLE_VALUE;
+      server_process = INVALID_HANDLE_VALUE;
+      loghandle = INVALID_HANDLE_VALUE;
+#else
+      server_pid = -1;
+#endif
+    } else if (force) {
+      /* Looks like we've already disconnected.  So the only thing to do
+       * is a "hard" kill of the server. */
+#ifdef WIN32_NATIVE
+      TerminateProcess(server_process, 0);
+      CloseHandle(server_process);
+      if (loghandle != INVALID_HANDLE_VALUE) {
+       CloseHandle(loghandle);
+      }
+      server_process = INVALID_HANDLE_VALUE;
+      loghandle = INVALID_HANDLE_VALUE;
 #else
-    kill(server_pid, SIGTERM);
-    waitpid(server_pid, NULL, WUNTRACED);
-    server_pid = - 1;
-#endif    
+      kill(server_pid, SIGTERM);
+      waitpid(server_pid, NULL, WUNTRACED);
+      server_pid = -1;
+#endif
+    }
   }
   client_has_hack = FALSE;
 }   
@@ -155,7 +183,7 @@ void client_kill_server()
 **************************************************************************/
 static void server_shutdown(void)
 {
-  client_kill_server();
+  client_kill_server(TRUE);
 }
                                                                                
 /**************************************************************** 
@@ -187,7 +215,7 @@ bool client_start_server(void)
 
   /* only one server (forked from this client) shall be running at a time */
   /* This also resets client_has_hack. */
-  client_kill_server();
+  client_kill_server(TRUE);
   
   if (!initialized) {
     atexit(server_shutdown);
@@ -339,7 +367,7 @@ bool client_start_server(void)
    * capabilities won't help us here... */ 
   if (!aconnection.used) {
     /* possible that server is still running. kill it */ 
-    client_kill_server();
+    client_kill_server(TRUE);
 
     append_output_window(_("Couldn't connect to the server."));
     append_output_window(_("We probably couldn't start it from here."));
@@ -469,7 +497,7 @@ void handle_single_want_hack_reply(bool 
     /* only output this if we started the server and we NEED hack */
     append_output_window(_("We can't take control of server, "
                          "attempting to kill it."));
-    client_kill_server();
+    client_kill_server(TRUE);
   }
 }
 
Index: client/connectdlg_common.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/connectdlg_common.h,v
retrieving revision 1.8
diff -p -u -r1.8 connectdlg_common.h
--- client/connectdlg_common.h  5 Feb 2005 07:15:36 -0000       1.8
+++ client/connectdlg_common.h  8 Aug 2005 19:25:51 -0000
@@ -20,7 +20,7 @@ Freeciv - Copyright (C) 2003 - The Freec
 #endif
 
 bool client_start_server(void);
-void client_kill_server(void);
+void client_kill_server(bool force);
 
 bool is_server_running(void);
 bool can_client_access_hack(void);
@@ -29,8 +29,6 @@ void send_client_wants_hack(const char *
 void send_start_saved_game(void);
 void send_save_game(char *filename);
 
-void disconnected_from_local_server(void);
-
 void set_ruleset(const char *ruleset);
 
 extern char player_name[MAX_LEN_NAME];
Index: client/gui-win32/connectdlg.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-win32/connectdlg.c,v
retrieving revision 1.38
diff -p -u -r1.38 connectdlg.c
--- client/gui-win32/connectdlg.c       1 Aug 2005 06:48:29 -0000       1.38
+++ client/gui-win32/connectdlg.c       8 Aug 2005 19:25:51 -0000
@@ -798,7 +798,7 @@ static LONG CALLBACK new_game_proc(HWND 
          popup_races_dialog(game.player_ptr);
          break;
        case ID_CANCEL:
-         client_kill_server();
+         client_kill_server(TRUE);
          ShowWindow(newgame_dlg,SW_HIDE);
          ShowWindow(start_dlg,SW_SHOWNORMAL);
          break;