Proposal: Breaking change to foam.net.node.Handler interface

[Mark Dittmer]

tl;dr: This email sketches a proposal to turn Handler.handle() into synchronous Handler.canHandleRequest() and asynchronous Handler.handleRequest(). If you use foam.net.node.Server and/or foam.net.node.Handler please respond with "This affects me!". Also, any feedback you have is much appreciated :-)

foam.net.node.Handler.handle() conflates two things:

1. Can this Handler handle this request? (synchronous response value);
2. Handling of the request (usually asynchronous flow).

This makes the interface difficult to decorate, because a decorator cannot ask its delegate whether or not the delegate should handle the request, but not ask it to handle it just yet. This is useful for, e.g., a caching decorator.

I propose that handle() become a canHandleRequest() ? handleRequest() : noop(). Handlers can be changed without touching foam.net.node.Server to start. Then we can port Server and add a deprecation warning to handle(). Then handle will be removed.

Thoughts on this change?

//Mark

[Braden Shepherdson]

+1

Decoration is power, and this makes decoration much more flexible.

Braden

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-di...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mike Carcasole]

Does this approach really help? When thinking about it, I keep comparing it to a DAO's put() method and we've never needed a canPut() on the interface to help decorate. I just worry that this could set a precedent for splitting async foo() methods up into a sync canFoo() and async doFoo().

What are you trying to accomplish with this change that can't be done by decorating the handle() method alone?

And is there an expectation that, if canHandle returns true, handleRequest always succeeds or can race conditions cause it to fail still?

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsub...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsub...@googlegroups.com.

[Mark Dittmer]

Responded inline.

On Mon, Sep 18, 2017 at 5:28 PM, Mike Carcasole <mikeca...@gmail.com> wrote:

Does this approach really help? When thinking about it, I keep comparing it to a DAO's put() method and we've never needed a canPut() on the interface to help decorate. I just worry that this could set a precedent for splitting async foo() methods up into a sync canFoo() and async doFoo().

What are you trying to accomplish with this change that can't be done by decorating the handle() method alone?

This PR fumbles through the idea in the absence of an upstream change. Without this (or a similar mechanism), the CacheHandler decorator kicks off the DAO query when it attempts to determine whether or not its delegate handles the request. Before this PR lands, the CacheHandler delivers cached results when they're available, but wasteful DAO queries get queued anyway (slowing down requests that are cache misses).

I'm totally open to, instead, rethinking how foam.net.node.Server delegates to handlers if a different design eliminates the need for this split. That said, caching requests seems like a pretty important request-handler decoration use case.

 

And is there an expectation that, if canHandle returns true, handleRequest always succeeds or can race conditions cause it to fail still?

To clarify, handlers may still error out (because async work may yield errors) after handle/canHandleRequest returns true. What returning true eliminates is the Server continuing to iterate over handlers, looking for one to take the request.

 

On Mon, Sep 18, 2017 at 2:34 PM, 'Braden Shepherdson' via foam-framework-discuss <foam-framework-discuss@googlegroups.com> wrote:

+1

Decoration is power, and this makes decoration much more flexible.

Braden

On Mon, Sep 18, 2017 at 1:04 PM Mark Dittmer <mark.s....@gmail.com> wrote:

tl;dr: This email sketches a proposal to turn Handler.handle() into synchronous Handler.canHandleRequest() and asynchronous Handler.handleRequest(). If you use foam.net.node.Server and/or foam.net.node.Handler please respond with "This affects me!". Also, any feedback you have is much appreciated :-)

foam.net.node.Handler.handle() conflates two things:

1. Can this Handler handle this request? (synchronous response value);
2. Handling of the request (usually asynchronous flow).

This makes the interface difficult to decorate, because a decorator cannot ask its delegate whether or not the delegate should handle the request, but not ask it to handle it just yet. This is useful for, e.g., a caching decorator.

I propose that handle() become a canHandleRequest() ? handleRequest() : noop(). Handlers can be changed without touching foam.net.node.Server to start. Then we can port Server and add a deprecation warning to handle(). Then handle will be removed.

Thoughts on this change?

//Mark

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsubscri...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsubscri...@googlegroups.com.

[Mike Carcasole]

I might not be following the full story here as I'm not super familiar with this code but I wonder if Server can be simplified to just take a single Handler instead of taking a bunch of handlers.

If Handler is an interface with a handle(req, res) method that returns a promise, you should be able to build a handler implementation that allows you to route a request to a static file handler, dao handler, or even another routing handler. This way, only the routing handler needs to be concerned about the URL and the rest of the handlers handle everything that's thrown at them. Now, your server doesn't know/care which handlers are coming into play because it just calls handle() on the handler it holds on to. This makes the the routing handler responsible for ensuring it doesn't call all handlers on a given request.

Would an approach like that help at all or am I missing something here?

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsub...@googlegroups.com.

[Adam Van Ymeren]

I agree with Mike, I don't think the router should be part of the HTTP server itself, but should be a strategy for Handler.  Make it an interface, and then compose it, generally a good strategy.  Plus then different routing strategies can be plugged in, allowing you to do your canHandle design in the confluence repository if you still chose to go that way.

I also don't like canHandle, but I see the problem you're facing and why you would need it.  I think instead of doing a round robin on all handlers to see who can handle a particular URL, handlers should just be bound to specific URLs, or URL prefixes, and they _have_ to handle anything that gets routed to them.  This is how servlets are mapped on Java EE servers

Responded inline.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsub...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsub...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mark Dittmer]

I'm going to begin working on this. Are there others using `foam.net.node.Server` that may be affected?

//Mark

Responded inline.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsubscri...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsubscri...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "foam-framework-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsubscri...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Mark Dittmer]

I'm hoping to incorporate the design suggestions. Here's a rough sketch:

Server owns a Router

Router binds Routes to Handlers with: void addRoute(Route, Handler)

Routes implement: bool matches(URL)

Handler interface remains the same; what to do with handle() returning false is an implementation detail of the Router

Default Router for default Server is a PathnamePrefixRouter that won't accept overlapping bindings (cannot register "/foo" and "/foo/bar"; compose Handlers if you want that). It will issue a 500 if a handler refuses to handle URLs that match its pathname prefix.

I think this incorporates the design feedback, and, in particular, does not demand canHandle() along with handle(). Strategies such as caching can be decorated on the Handlers before they are added to a Router.

Thoughts?

//Mark

On Tue, Oct 31, 2017 at 8:58 PM, Adam Van Ymeren <ad...@vany.ca> wrote:

There are unresolved design questions. Are you planning to address them or stick to your original design?

[Mike Carcasole]

Does Router implement Handler?

To unsubscribe from this group and stop receiving emails from it, send an email to foam-framework-discuss+unsub...@googlegroups.com.

[Mark Dittmer]

My thinking was, no. A Server has a single Router. The Router (not the Server) is responsible for managing Route<-->Handler bindings (which the Server used to do implicitly by iterating over an array of Handlers).

On the other hand, Routers could be composed (e.g., tree structure for pathname prefix router) if they implement Handler, because we could do router1.addRoute(baseRouteForRouter2, router2). Is that what you had in mind?

//Mark

[Mike Carcasole]

Yeah having Router implement Handler was what I had in mind. I think that approach will be easier to decorate and compose. So a server has a single Handler and if that handler happens to be a Router implementation of Handler, it routes to whatever handler is wants, which can route some more if that handler is a Router.