# HG changeset patch
# User Adam Chlipala <adam@chlipala.net>
# Date 1397603569 14400
# Node ID bddee3af70c4062c84ba31469c40bce4107ad799
# Parent  a1d3fbdcc8974fdb06c698dd95b2fc8554732444
Tweaking uw_commit() logic, partly to fix a resource clean-up bug on SQL serialization failures

diff -r a1d3fbdcc897 -r bddee3af70c4 doc/manual.tex
--- a/doc/manual.tex	Mon Feb 24 09:10:31 2014 +0000
+++ b/doc/manual.tex	Tue Apr 15 19:12:49 2014 -0400
@@ -2464,7 +2464,7 @@
   \end{verbatim}
   All side effects in Ur/Web programs need to be compatible with transactions, such that any set of actions can be undone at any time.  Thus, you should not perform actions with non-local side effects directly; instead, register handlers to be called when the current transaction is committed or rolled back.  The arguments here give an arbitary piece of data to be passed to callbacks, a function to call on commit, a function to call on rollback, and a function to call afterward in either case to clean up any allocated resources.  A rollback handler may be called after the associated commit handler has already been called, if some later part of the commit process fails.  A free handler is told whether the runtime system expects to retry the current page request after rollback finishes.
 
-  Any of the callbacks may be \texttt{NULL}.  To accommodate some stubbornly non-transactional real-world actions like sending an e-mail message, Ur/Web treats \texttt{NULL} \texttt{rollback} callbacks specially.  When a transaction commits, all \texttt{commit} actions that have non-\texttt{NULL} rollback actions are tried before any \texttt{commit} actions that have \texttt{NULL} rollback actions.  Thus, if a single execution uses only one non-transactional action, and if that action never fails partway through its execution while still causing an observable side effect, then Ur/Web can maintain the transactional abstraction.
+  Any of the callbacks may be \texttt{NULL}.  To accommodate some stubbornly non-transactional real-world actions like sending an e-mail message, Ur/Web treats \texttt{NULL} \texttt{rollback} callbacks specially.  When a transaction commits, all \texttt{commit} actions that have non-\texttt{NULL} rollback actions are tried before any \texttt{commit} actions that have \texttt{NULL} rollback actions.  Furthermore, an SQL \texttt{COMMIT} is also attempted in between the two phases, so the nicely transactional actions have a chance to influence whether data are committed to the database, while \texttt{NULL}-rollback actions only get run in the first place after committing data.  The reason for all this is that it is \emph{expected} that concurrency interactions will cause database commits to fail in benign ways that call for transaction restart.  A truly non-undoable action should only be run after we are sure the database transaction will commit.
 
   When a request handler ends with multiple pending transactional actions, their handlers are run in a first-in-last-out stack-like order, wherever the order would otherwise be ambiguous.
 
diff -r a1d3fbdcc897 -r bddee3af70c4 src/c/urweb.c
--- a/src/c/urweb.c	Mon Feb 24 09:10:31 2014 +0000
+++ b/src/c/urweb.c	Tue Apr 15 19:12:49 2014 -0400
@@ -3304,32 +3304,58 @@
         }
       }
 
+  if (ctx->transaction_started) {
+    int code = ctx->app->db_commit(ctx);
+
+    if (code) {
+      if (ctx->client)
+        release_client(ctx->client);
+
+      if (code == -1) {
+        // This case is for a serialization failure, which is not really an "error."
+        // The transaction will restart, so we should rollback any transactionals
+        // that triggered above.
+
+        for (i = ctx->used_transactionals-1; i >= 0; --i)
+          if (ctx->transactionals[i].rollback != NULL)
+            ctx->transactionals[i].rollback(ctx->transactionals[i].data);
+
+        for (i = ctx->used_transactionals-1; i >= 0; --i)
+          if (ctx->transactionals[i].free)
+            ctx->transactionals[i].free(ctx->transactionals[i].data, 1);
+
+	return 1;
+      }
+
+      for (i = ctx->used_transactionals-1; i >= 0; --i)
+        if (ctx->transactionals[i].free)
+          ctx->transactionals[i].free(ctx->transactionals[i].data, 0);
+
+      uw_set_error_message(ctx, "Error running SQL COMMIT");
+      return 0;
+    }
+  }
+
   for (i = ctx->used_transactionals-1; i >= 0; --i)
     if (ctx->transactionals[i].rollback == NULL)
       if (ctx->transactionals[i].commit) {
         ctx->transactionals[i].commit(ctx->transactionals[i].data);
         if (uw_has_error(ctx)) {
-          uw_rollback(ctx, 0);
+           if (ctx->client)
+             release_client(ctx->client);
+
+           for (i = ctx->used_transactionals-1; i >= 0; --i)
+             if (ctx->transactionals[i].rollback != NULL)
+               ctx->transactionals[i].rollback(ctx->transactionals[i].data);
+
+           for (i = ctx->used_transactionals-1; i >= 0; --i)
+             if (ctx->transactionals[i].free)
+               ctx->transactionals[i].free(ctx->transactionals[i].data, 0);
+
           return 0;
         }
       }
 
-  if (ctx->transaction_started) {
-    int code = ctx->app->db_commit(ctx);
-
-    if (code) {
-      if (code == -1)
-	return 1;
-
-      for (i = ctx->used_transactionals-1; i >= 0; --i)
-	if (ctx->transactionals[i].free)
-	  ctx->transactionals[i].free(ctx->transactionals[i].data, 0);
-
-      uw_set_error_message(ctx, "Error running SQL COMMIT");
-      return 0;
-    }
-  }
-
   for (i = 0; i < ctx->used_deltas; ++i) {
     delta *d = &ctx->deltas[i];
     client *c = find_client(d->client);