Changeset: 9d2c0c613846 for MonetDB URL: https://dev.monetdb.org/hg/MonetDB?cmd=changeset;node=9d2c0c613846 Modified Files: common/utils/msabaoth.c tools/merovingian/daemon/client.c tools/merovingian/daemon/controlrunner.c tools/merovingian/daemon/forkmserver.c tools/merovingian/daemon/handlers.c tools/merovingian/daemon/merovingian.c tools/merovingian/daemon/merovingian.h tools/merovingian/daemon/multiplex-funnel.c Branch: Nov2019 Log Message:
Fixed a bunch of race conditions + some code cleanup. When an mserver crashes, it leaves behind the .started file. If a new mserver is then started, it first locks the database and then, later, removes the .started file. In the intervening time, the database seems up (locked and .started file present) but isn't yet, causing errors for new connections. Apparently other things could go wrong as well causing the internal state of monetdbd to become confused and causing messages about an "impossible state". We now use a per database lock inside monetdbd when checking whether a server is up. This means that if there are multiple clients connecting, they all have to wait until the server is back up. We also remove the .started file before starting the server. The only drawback is that the internal list of databases past and present is never cleaned up. If this becomes a problem, we need to implement a fix for that. This needs more testing. diffs (truncated from 1068 to 300 lines): diff --git a/common/utils/msabaoth.c b/common/utils/msabaoth.c --- a/common/utils/msabaoth.c +++ b/common/utils/msabaoth.c @@ -611,7 +611,7 @@ msab_getSingleStatus(const char *pathbuf */ snprintf(buf, sizeof(buf), "%s/%s/%s", pathbuf, dbname, _sabaoth_internal_uuid); - if (stat(buf, &statbuf) != -1) { + if (stat(buf, &statbuf) == 0) { /* database has the same process signature as ours, which * means, it must be us, rely on the uplog state */ snprintf(log, sizeof(log), "%s/%s/%s", pathbuf, dbname, UPLOGFILE); @@ -635,7 +635,7 @@ msab_getSingleStatus(const char *pathbuf (void)fclose(f); } } else if (snprintf(buf, sizeof(buf), "%s/%s/%s", pathbuf, dbname, ".gdk_lock"), - ((fd = MT_lockf(buf, F_TEST, 4, 1)) == -2)) { + ((fd = MT_lockf(buf, F_TLOCK, 4, 1)) == -2)) { /* Locking failed; this can be because the lockfile couldn't * be created. Probably there is no Mserver running for * that case also. @@ -644,15 +644,18 @@ msab_getSingleStatus(const char *pathbuf } else if (fd == -1) { /* file is locked, so mserver is running, see if the database * has finished starting */ - snprintf(buf, sizeof(buf), "%s/%s/%s", - pathbuf, dbname, STARTEDFILE); + snprintf(buf, sizeof(buf), "%s/%s/%s", pathbuf, dbname, STARTEDFILE); if (stat(buf, &statbuf) == -1) { sdb->state = SABdbStarting; } else { sdb->state = SABdbRunning; } } else { - /* file is not locked, check for a crash in the uplog */ + /* file was not locked (we just locked it), check for a crash + * in the uplog */ + snprintf(log, sizeof(log), "%s/%s/%s", pathbuf, dbname, STARTEDFILE); + /* just to be sure, remove the .started file */ + (void) remove(log); /* may fail, that's fine */ snprintf(log, sizeof(log), "%s/%s/%s", pathbuf, dbname, UPLOGFILE); if ((f = fopen(log, "r")) != NULL) { (void)fseek(f, -1, SEEK_END); @@ -669,28 +672,27 @@ msab_getSingleStatus(const char *pathbuf /* no uplog, so presumably never started */ sdb->state = SABdbInactive; } + MT_lockf(buf, F_ULOCK, 4, 1); + close(fd); } snprintf(buf, sizeof(buf), "%s/%s/%s", pathbuf, dbname, MAINTENANCEFILE); - sdb->locked = stat(buf, &statbuf) != -1; + sdb->locked = stat(buf, &statbuf) == 0; /* add scenarios that are supported */ sdb->scens = NULL; snprintf(buf, sizeof(buf), "%s/%s/%s", pathbuf, dbname, SCENARIOFILE); if ((f = fopen(buf, "r")) != NULL) { sablist* np = NULL; - while (fgets(data, 8095, f) != NULL) { + while (fgets(data, (int) sizeof(data), f) != NULL) { if (*data != '\0' && data[strlen(data) - 1] == '\n') data[strlen(data) - 1] = '\0'; if (sdb->scens == NULL) { - sdb->scens = malloc(sizeof(sablist)); - sdb->scens->val = strdup(data); - sdb->scens->next = NULL; - np = sdb->scens; + np = sdb->scens = malloc(sizeof(sablist)); } else { np = np->next = malloc(sizeof(sablist)); - np->val = strdup(data); - np->next = NULL; } + np->val = strdup(data); + np->next = NULL; } (void)fclose(f); } @@ -700,19 +702,16 @@ msab_getSingleStatus(const char *pathbuf snprintf(buf, sizeof(buf), "%s/%s/%s", pathbuf, dbname, CONNECTIONFILE); if ((f = fopen(buf, "r")) != NULL) { sablist* np = NULL; - while (fgets(data, 8095, f) != NULL) { + while (fgets(data, (int) sizeof(data), f) != NULL) { if (*data != '\0' && data[strlen(data) - 1] == '\n') data[strlen(data) - 1] = '\0'; if (sdb->conns == NULL) { - sdb->conns = malloc(sizeof(sablist)); - sdb->conns->val = strdup(data); - sdb->conns->next = NULL; - np = sdb->conns; + np = sdb->conns = malloc(sizeof(sablist)); } else { np = np->next = malloc(sizeof(sablist)); - np->val = strdup(data); - np->next = NULL; } + np->val = strdup(data); + np->next = NULL; } (void)fclose(f); } diff --git a/tools/merovingian/daemon/client.c b/tools/merovingian/daemon/client.c --- a/tools/merovingian/daemon/client.c +++ b/tools/merovingian/daemon/client.c @@ -44,7 +44,7 @@ struct threads { struct threads *next; pthread_t tid; - volatile char dead; + volatile bool dead; }; struct clientdata { int sock; @@ -85,7 +85,7 @@ handleClient(void *data) free(data); fdin = socket_rstream(sock, "merovingian<-client (read)"); if (fdin == 0) { - self->dead = 1; + self->dead = true; return(newErr("merovingian-client inputstream problems")); } fdin = block_stream(fdin); @@ -93,7 +93,7 @@ handleClient(void *data) fout = socket_wstream(sock, "merovingian->client (write)"); if (fout == 0) { close_stream(fdin); - self->dead = 1; + self->dead = true; return(newErr("merovingian-client outputstream problems")); } fout = block_stream(fout); @@ -137,7 +137,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } buf[sizeof(buf) - 1] = '\0'; @@ -158,7 +158,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } @@ -174,7 +174,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } algo = passwd + 1; @@ -185,7 +185,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } *s = 0; @@ -196,7 +196,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } @@ -211,7 +211,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } @@ -229,7 +229,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } else { *s = '\0'; @@ -243,7 +243,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(newErr("client %s specified no database", host)); } @@ -253,7 +253,7 @@ handleClient(void *data) control_handleclient(host, sock, fdin, fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(NO_ERR); } @@ -283,7 +283,7 @@ handleClient(void *data) mnstr_flush(fout); close_stream(fout); close_stream(fdin); - self->dead = 1; + self->dead = true; return(e); } stat = top; @@ -295,7 +295,7 @@ handleClient(void *data) { multiplexAddClient(top->dbname, sock, fout, fdin, host); msab_freeStatus(&top); - self->dead = 1; + self->dead = true; return(NO_ERR); } @@ -324,7 +324,7 @@ handleClient(void *data) close_stream(fout); close_stream(fdin); msab_freeStatus(&top); - self->dead = 1; + self->dead = true; return(e); } @@ -395,13 +395,13 @@ handleClient(void *data) close_stream(fdin); Mfprintf(stdout, "starting a proxy failed: %s\n", e); msab_freeStatus(&top); - self->dead = 1; + self->dead = true; return(e); } } msab_freeStatus(&top); - self->dead = 1; + self->dead = true; return(NO_ERR); } @@ -624,7 +624,7 @@ acceptConnections(int sock, int usock) } data->sock = msgsock; data->isusock = isusock; - p->dead = 0; + p->dead = false; data->self = p; data->challenge[31] = '\0'; generateSalt(data->challenge, 31); diff --git a/tools/merovingian/daemon/controlrunner.c b/tools/merovingian/daemon/controlrunner.c --- a/tools/merovingian/daemon/controlrunner.c +++ b/tools/merovingian/daemon/controlrunner.c @@ -355,9 +355,12 @@ static void ctl_handle_client( dp = _mero_topdp->next; /* don't need the console/log */ while (dp != NULL) { if (dp->type == MERODB && strcmp(dp->dbname, q) == 0) { + if (dp->pid <= 0) { + dp = NULL; + /* unlock happens below */ + break; + } if (strcmp(p, "stop") == 0) { - pid_t pid = dp->pid; - char *dbname = strdup(dp->dbname); mtype type = dp->type; pthread_mutex_unlock(&_mero_topdp_lock); /* Try to shutdown the profiler before the DB. @@ -366,10 +369,12 @@ static void ctl_handle_client( * other words: ignore any errors that shutdown_profiler * may have encountered. */ - shutdown_profiler(dbname, &stats); + shutdown_profiler(dp->dbname, &stats); if (stats != NULL) msab_freeStatus(&stats); - terminateProcess(pid, dbname, type, 1); + pthread_mutex_lock(&dp->fork_lock); + terminateProcess(dp, type); + pthread_mutex_unlock(&dp->fork_lock); Mfprintf(_mero_ctlout, "%s: stopped " "database '%s'\n", origin, q); } else { diff --git a/tools/merovingian/daemon/forkmserver.c b/tools/merovingian/daemon/forkmserver.c _______________________________________________ checkin-list mailing list checkin-list@monetdb.org https://www.monetdb.org/mailman/listinfo/checkin-list