Fork me on GitHub
#pedestal
<
2017-03-24
>
alanspringfield06:03:28

I would go with 1, that way you don't have to update the Vase docs everytime the Pedestal docs changes

deg11:03:28

1, with a short roadmap doc.

manutter5112:03:28

To follow up on my earlier issue, my fairly new pedestal service started throwing exceptions when starting the REPL, complaining that No matching ctor found for class io.pedestal.http.ring_middlewares$response_fn_adapter$fn__11673. After numerous lein clean, deleting and re-loading dependencies in ~/.m2, and other suggestions, I was still getting the error, but today it stopped happening.

mtnygard12:03:57

@manutter51 I hate when bugs just go away on their own. That means they can come back on their own, too.

manutter5112:03:27

I didn’t have time this morning to fully test it, but the thing that seems to have done the trick is moving the session configuration to the service map in service.clj. I thought I had done that earlier, but I realized I had added it to the map in server.clj that run-dev merges with the main service map.

manutter5112:03:57

moving the session setting to the right map seems to have fixed the issue

manutter5112:03:25

though I have no clue why that particular cause would have that particular effect.

kenny19:03:03

Is the only way to exit the interceptor chain by throwing an exception? For example, I have 3 interceptors for my route and interceptor1 decides that the request should not be completed right now (maybe it was an invalid request, user not authorized, the server has other processing duties, etc.). Does interceptor1 have to throw an exception in order to stop subsequent interceptors from running, or is there another way?

ddeaguiar19:03:48

@kenny you can also have your interceptor return a response

ddeaguiar19:03:26

Once an interceptor returns a response, the execution of downstream interceptor :enter fns is terminated

ddeaguiar19:03:26

@kenny not return a response, assoc a response onto the context

ddeaguiar19:03:36

so to state it correctly, once an interceptor assoc’s a response to the context, the execution of downstream interceptor :enter fns is terminated

kenny19:03:58

Ah gotcha. Unless I missed it, I don't believe that was mentioned in the docs.

kenny19:03:36

Is the :leave called of subsequent interceptors?

ddeaguiar19:03:22

so the :leave fn is called for all interceptors that were already executed

kenny19:03:49

Makes sense.

ddeaguiar19:03:42

Hmm, I think you’re right about the docs (http://pedestal.io/reference/interceptors), they don’t mention this.

kenny19:03:00

Yeah. I'd think it would go under "Interceptor Return Values"

kenny19:03:25

Not sure if you'd ever want to do this but is it possible to terminate the :leave execution as well?

kenny19:03:50

Wait I don't think associng a :response onto the context stops the interceptor flow. I just tried it and my second interceptor got called

ddeaguiar19:03:44

hmm, I do this all the time

kenny19:03:55

The second interceptor does not get called if I wrap the context with terminate.

kenny19:03:14

Yeah the second interceptor is definitely getting called even though the context has a :response assoc'ed on it.

kenny19:03:26

I think that makes sense though.

kenny19:03:56

Explicit termination is much better than implicit. Usually 😉

ddeaguiar19:03:08

The interceptor after the one which assoc’d the response?

kenny19:03:32

(def echo
  {:name :echo
   :enter
         (fn [context]
           (clojure.pprint/pprint context)
           (let [request (:request context)
                 response (ok context)]
             (assoc context :response response)))})

(def echo2
  {:name  :echo2
   :enter (fn [context]
            (println "echo2")
            context)})

(def routes
  (route/expand-routes
    #{["/foo" :post [echo echo2] :route-name :foo]}))

kenny19:03:43

echo2 is printed

ddeaguiar19:03:07

hrmm, interesting

kenny19:03:47

If instead echo calls terminate:

(def echo
  {:name :echo
   :enter
         (fn [context]
           (clojure.pprint/pprint context)
           (let [request (:request context)
                 response (ok context)]
             (chain/terminate (assoc context :response response))))})
then echo2 is not printed.

ddeaguiar19:03:10

I’m going to play with this because it doesn’t behave how I expected. Perhaps things have changed!

kenny19:03:41

You may be getting unnecessary database load if the chain isn't actually terminating 🙂

ddeaguiar20:03:56

@kenny are you using a custom chain provider?

ddeaguiar20:03:22

or a default setup using lein new pedestal-service

kenny20:03:24

This is the ns I'm messing with: http://pastebin.com/eMTkYEwu

ddeaguiar20:03:12

@kenny, so this example (https://gist.github.com/ddeaguiar/74eed4784848e49fa83179a2086aa9ea) behaves as I described. Because the :io.pedestal.interceptor.chain/terminators coll on the context has a single predicate which is set by the default servlet chain provider. https://github.com/pedestal/pedestal/blob/41337e41ae773f3c17baeb1c244be13ddf6c28db/service/src/io/pedestal/http/impl/servlet_interceptor.clj#L231

ddeaguiar20:03:56

Your example appears to use the default servlet chain provider as well

ddeaguiar20:03:14

In my example, when you request you’ll see this in the logs

16:01:31.660 [qtp2141354770-51] INFO  terminator.service - {:msg "In Terminator", :line 23}                                  │
16:01:31.663 [qtp2141354770-51] INFO  terminator.service - {:terminators (#function[io.pedestal.http.impl.servlet-interceptor│
16:01:31.663 [qtp2141354770-51] DEBUG io.pedestal.interceptor.chain - {:in check-terminators, :terminate? true, :execution-id│
16:01:31.664 [qtp2141354770-51] DEBUG io.pedestal.interceptor.chain - {:in leave-all, :execution-id 19, :line 239}           │
16:01:31.666 [qtp2141354770-51] DEBUG io.pedestal.interceptor.chain - {:interceptor :terminator.service/terminator, :stage :l│
16:01:31.666 [qtp2141354770-51] INFO  terminator.service - {:msg "Leaving Terminator", :line 29}

ddeaguiar20:03:26

I added the following line to my logback.xml file to log what the interceptor chain is doing:

<logger name="io.pedestal.interceptor.chain" level="DEBUG" />

kenny20:03:36

I understand why

ddeaguiar20:03:47

btw, I also recommend using fully qualified symbols in your routes

kenny20:03:27

I copied this function from the guide.

(defn response [status body & {:as headers}]
  {:status status :body body :headers headers})
If you don't provide headers (e.g. (response 200 "foo")) then it returns {:status 200, :body "foo", :headers {}} which is not a ring response.
(ring.util.response/response? (response 200 "foo"))
=> false

kenny20:03:44

Instead you need to do this:

(defn response [status body & {:as headers}]
  {:status status :body body :headers (or headers {})})

ddeaguiar20:03:52

BTW, I really appreciate you asking this question. I’ve got a much deeper understanding of chain providers now!

kenny20:03:09

Once you do that the interceptor chain is terminated when a :response is assoc'ed onto the context.

kenny20:03:14

Same here 🙂

ddeaguiar20:03:44

hrm I think Pedestal needs better response checking

ddeaguiar20:03:08

like if something’s assoc’d to the context as :response but it’s not a valid response, then it should at least log a warning

ddeaguiar20:03:36

So, everything that drives Pedestal can be found in the context

ddeaguiar20:03:46

If you create a custom chain provider and don’t need terminators, then don’t add io.pedestal.interceptor.chain/terminators to the context

ddeaguiar20:03:27

Notice how it lacks some keys

ddeaguiar20:03:08

That’s part of a custom chain provider bound to direct-jetty-provider

ddeaguiar20:03:23

So the nuance is, if you’re using a the default servlet chain provider, or another chain provider which terminates when a response is assoc’d to the context, then you don’t need to call terminate

ddeaguiar20:03:49

I’ll add a quote from ohpauleez > please don't terminate unless the situation is dire

ddeaguiar20:03:06

meaning, don’t explicitly call terminate

kenny20:03:38

Really the only time you'd want to terminate is after you have returned a response.

ddeaguiar20:03:40

and that’s handled for you

ddeaguiar20:03:16

So another thing to remember is that handlers are interceptors

ddeaguiar20:03:25

So all of this makes sense, in retrospect

ddeaguiar20:03:55

but we always put handlers at the end of the interceptor chain

ddeaguiar20:03:06

I’ve actually never tried to put them anywhere else

kenny20:03:06

Not sure if it'd ever make sense to put them anywhere else

ddeaguiar20:03:52

It doesn’t. Why I’ve never done it

ddeaguiar20:03:11

hehe, this was a great rabbit hole!

kenny20:03:47

On a side note, this came up because I was considering things like authentication. It's surprising that by default the only Pedestal authentication library geheimtur uses exceptions for control flow: https://github.com/propan/geheimtur/blob/master/src/geheimtur/interceptor.clj#L14

ddeaguiar20:03:34

other auth impls do similar things

ddeaguiar20:03:41

Take buddy-auth as an example

kenny20:03:43

But it is terrible for perf

kenny20:03:28

Just because other libs do it doesn't mean it was the right decision. IMO exceptions should only be used for exceptional things, not logic.

ddeaguiar20:03:40

which, btw, I created a sample demonstrating it’s usage: https://github.com/pedestal/pedestal/pull/503

kenny20:03:47

https://github.com/pedestal/pedestal/tree/master/samples/fast-pedestal > Avoid throwing exceptions (especially in interceptors)

ddeaguiar20:03:03

yeah, I tend to agree with you

kenny20:03:43

One of my fav articles on exception performance.

ddeaguiar20:03:43

So, for example, with buddy-auth, you don’t have to throw, you could just return a response as well

ddeaguiar20:03:56

My sample throws because that’s the default usage

kenny20:03:13

Right but because they have throwing as a default usage it hints others that the use of exceptions is acceptable for control flow.

ddeaguiar20:03:05

So when I write interceptors for things like validation, I short-circuit by returning a response

kenny20:03:10

And although it may work, it certainly isn't performant and there loads of articles about why exceptions shouldn't be used for that purpose.

kenny20:03:19

Yeah, that's exactly what I would do.

kenny20:03:31

Anyway, the docs definitely need to be updated.

ddeaguiar20:03:12

Thanks for the article link. I’ll check that out later

kenny20:03:12

It's long but I think it's valuable knowledge 🙂

kenny20:03:58

I'll open an issue in pedestal-docs. If I have time this weekend I'll submit a PR.

kenny20:03:07

Ah, looks like you already did 🙂

ddeaguiar20:03:23

hehe yeah. Feel free to pick it up