Fix ownership on 'NextCallback'

One big drawback of not passing the 'NextCallback' ownership forward is that it cannot be called after the callee has returned (e.g. in a thread or asynchronously). Make it owned across the whole 'perform_routing' recursion.
valum-framework · Mar 10, 2017 · 4605b4a · 4605b4a
1 parent e549e1b
commit 4605b4a
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 13 deletions.
diff --git a/examples/modular-app/admin-router.vala b/examples/modular-app/admin-router.vala
@@ -28,7 +28,7 @@ public class AdminRouter : Router {
 	/**
 	 * Verify the user credentials and perform an authentication.
 	 */
-	public bool authenticate (Request req, Response res, NextCallback next) throws Error {
+	public bool authenticate (Request req, Response res, owned NextCallback next) throws Error {
 		string @value;
 		req.lookup_signed_cookie ("session", ChecksumType.SHA256, "impossible to break".data, out @value);
 		if (@value == "admin") {
@@ -65,7 +65,7 @@ public class AdminRouter : Router {
 	/**
 	 * Restricted content.
 	 */
-	public bool view (Request req, Response res, NextCallback next, Context ctx) throws Error {
+	public bool view (Request req, Response res, owned NextCallback next, Context ctx) throws Error {
 		res.headers.set_content_type ("text/plain", null);
 		return res.expand_utf8 ("Hello admin!");
 	}

diff --git a/examples/modular-app/user-router.vala b/examples/modular-app/user-router.vala
@@ -24,7 +24,7 @@ public class UserRouter : Router {
 		get ("/user/<int:id>", view);
 	}
 
-	public bool view (Request req, Response res, NextCallback next, Context ctx) throws Error {
+	public bool view (Request req, Response res, owned NextCallback next, Context ctx) throws Error {
 		res.headers.set_content_type ("text/plain", null);
 		return res.expand_utf8 ("Hello, user %s!".printf (ctx["id"].get_string ()));
 	}

diff --git a/src/valum/valum-forward.vala b/src/valum/valum-forward.vala
@@ -25,7 +25,7 @@ namespace Valum {
 	 * continuation.
 	 */
 	[Version (since = "0.3")]
-	public bool forward (Request req, Response res, NextCallback next) throws Error {
+	public bool forward (Request req, Response res, owned NextCallback next) throws Error {
 		return next ();
 	}
 }
diff --git a/src/valum/valum-router.vala b/src/valum/valum-router.vala
@@ -346,7 +346,7 @@ namespace Valum {
 		private bool perform_routing (SequenceIter<Route> routes,
 		                              Request req,
 		                              Response res,
-		                              NextCallback next,
+		                              owned NextCallback next,
 		                              Context context) throws Informational,
 		                                                      Success,
 		                                                      Redirection,
@@ -362,7 +362,7 @@ namespace Valum {
 						if (node.next ().is_end ()) {
 							return next ();
 						} else {
-							return perform_routing (node.next (), req, res, next, local_context);
+							return perform_routing (node.next (), req, res, (owned) next, local_context);
 						}
 					}, local_context);
 				}

diff --git a/src/valum/valum.vala b/src/valum/valum.vala
@@ -64,10 +64,15 @@ namespace Valum {
 	 *         otherwise 'false'
 	 */
 	[Version (since = "0.1")]
-	public delegate bool HandlerCallback (Request      req,
-	                                      Response     res,
-	                                      NextCallback next,
-	                                      Context      context) throws Error;
+	public delegate bool HandlerCallback (Request req,
+	                                      Response res,
+	                                      owned NextCallback next,
+	                                      Context context) throws Informational,
+	                                                              Success,
+	                                                              Redirection,
+	                                                              ClientError,
+	                                                              ServerError,
+	                                                              Error;
 
 	/**
 	 * Define a type of {@link Valum.HandlerCallback} that forward a generic
@@ -76,7 +81,7 @@ namespace Valum {
 	[Version (since = "0.3")]
 	public delegate bool ForwardCallback<T> (Request      req,
 	                                         Response     res,
-	                                         NextCallback next,
+	                                         owned NextCallback next,
 	                                         Context      context,
 	                                         T            @value) throws Error;
 

diff --git a/tests/router-test.vala b/tests/router-test.vala
@@ -1015,13 +1015,39 @@ public int main (string[] args) {
 		var request = new Request (null, "GET", new Soup.URI ("http://localhost/"));
 		var response = new Response (request, Soup.Status.NOT_FOUND);
 
+		assert (router.handle (request, response));
+	});
+
+	Test.add_func ("/router/next_in_thread", () => {
+		var router = new Router ();
+		Thread<bool>? thread = null;
+
+		router.get ("/", (req, res, next) => {
+			thread = new Thread<bool> (null, () => {
+				try {
+					return next ();
+				} catch (Error err) {
+					assert_not_reached ();
+				}
+			});
+			return true;
+		});
+
+		router.get ("/", (req, res) => {
+			return true;
+		});
+
+		var request = new Request (null, "GET", new Soup.URI ("http://localhost/"));
+		var response = new Response (request, Soup.Status.NOT_FOUND);
+
 		try {
-			router.handle (request, response);
+			assert (router.handle (request, response));
 		} catch (Error err) {
 			assert_not_reached ();
 		}
 
-		assert (418 == response.status);
+		assert (thread != null);
+		assert (thread.join ());
 	});
 
 	Test.add_func ("/router/next/not_found", () => {