`destroy` VS `contextDestroyed` about lein-ring HOT 9 OPEN

Basically, if I'm understanding correctly by the time .contextDestroyed has been called, it's too late to do anything meaningful - even something as trivial as logging.

Sorry, I'm afraid I'm not following. Why does the listener function care whether or not the servlet exists? It's just a shim for the handler.

from lein-ring.

Sorry, I'm afraid I'm not following. Why does the listener function care whether or not the servlet exists? It's just a shim for the handler.

I can't answer that with any form of confidence or authority...all I know, is that the code in our :destroy fns doesn't run on un/re-deploy, and since Friday I also know (from the docs) that the kind of work we're doing (cleanup/persist-state) should be done in the Sevlet::destroy method. I do have a local copy of lein-ring (for #217), so I'm going to try and implement this today, and see what happens. Will keep you posted...

from lein-ring.

I can confirm that overriding the .destroy method on the Servlet solves our issue - i.e. all the expected logging is now visible, and each component's state is persisted as well. Here is the modified compile-servlet:

(defn compile-servlet [project]
  (let [servlet-ns  (symbol (servlet-ns project))
        destroy-sym (get-in project [:ring :destroy])]
    (compile-form project servlet-ns
      `(do (ns ~servlet-ns
             (:gen-class
               :extends javax.servlet.http.HttpServlet
               :exposes-methods {~'destroy ~'superDestroy})  ;; <===
             (:import javax.servlet.http.HttpServlet))
           (def ~'service-method)
           (defn ~'-service [servlet# request# response#]
             (~'service-method servlet# request# response#))
           (defn ~'-destroy [this#]     ;; <===
             ~(if destroy-sym
                `(~(generate-resolve destroy-sym)))
             (. this# ~'superDestroy))) ;; <===
      :print-meta true)))

I guess, in an ideal situation lein-ring would offer the following (self-explanatory) options:

1. servlet-init
2. servlet-destroy
3. context-init
4. context-destroyed (notice the past tense)

Since, I presume that backwards compatibility is desired, and the :init/:destroy keys already refer to the context, realistically the only viable option is to simply add support for :servlet-init/destroy.

from lein-ring.

Yep, keeping compatibility is the best option. If you're having issues that are solved with overriding destroy, then that's likely reason enough to add a couple more configuration keys to Lein-Ring to override the init and destroy servlet methods.

from lein-ring.

Agreed...would you like a PR? I have no problem working on it, but I would prefer that we merge #217 first.

from lein-ring.

Sure. I had 20 minutes or so spare, so it's now merged.

from lein-ring.

Great thanks 👍 . Unfortunately, because of the manual merge, the new PR I want to open shows 16 commits ahead, even though there is a single new commit and file changed (a few lines actually). Will you be ok keeping the last commit only, if I open it like that?

from lein-ring.

Why not just reset to master and cherry pick the single commit you want?

from lein-ring.

Thanks for the idea - see #219

from lein-ring.