From 4605b4aaaa6d71dabb8c49fca9b3a993f79f4d20 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Sat, 27 Aug 2016 13:25:33 -0400 Subject: [PATCH] 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. --- examples/modular-app/admin-router.vala | 4 ++-- examples/modular-app/user-router.vala | 2 +- src/valum/valum-forward.vala | 2 +- src/valum/valum-router.vala | 4 ++-- src/valum/valum.vala | 15 ++++++++----- tests/router-test.vala | 30 ++++++++++++++++++++++++-- 6 files changed, 44 insertions(+), 13 deletions(-) diff --git a/examples/modular-app/admin-router.vala b/examples/modular-app/admin-router.vala index 8e711cbba..4b1ae9d4a 100644 --- 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 index 7ad74e4fd..336e430b3 100644 --- a/examples/modular-app/user-router.vala +++ b/examples/modular-app/user-router.vala @@ -24,7 +24,7 @@ public class UserRouter : Router { get ("/user/", 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 index c38bcceec..738f8e6f8 100644 --- 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 index 7634f9a02..4b918704e 100644 --- a/src/valum/valum-router.vala +++ b/src/valum/valum-router.vala @@ -346,7 +346,7 @@ namespace Valum { private bool perform_routing (SequenceIter 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 index 824f679a0..07f165e1e 100644 --- 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 (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 index 78c739f0b..9cec7d7b4 100644 --- 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? thread = null; + + router.get ("/", (req, res, next) => { + thread = new Thread (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", () => {