`destroy` VS `contextDestroyed`

[dpiliouras]

Before i say anything, please bear in mind that I am not a Servlet container expert. I'm purely going off of an entire day's worth of research, while trying to understand why our destroy fns are not being called.

Per the .destroy docs:

This method gives the servlet an opportunity to clean up any resources that are being held (for example, memory, file handles, threads) and make sure that any persistent state is synchronized with the servlet's current state in memory.

Contrast that with the .contextDestroyed docs:

All servlets and filters will have been destroyed before any ServletContextListeners are notified of context destruction.

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. Here is a related SO discussion.

A typical destroy fn for us looks something like this:

(defn tomcat-destroy []
  (when (realized? SYSTEM) ;; promise delivered on system init
    (ig/halt! @SYSTEM) ;; certain components write to disk on `ig/halt-key!`
    (l/trace "Halted system")
    (shutdown-agents)))

As you can see, our use-case aligns perfectly with the Servlet::destroy docs. So again, with my somewhat limited understanding, i suspect that instead of implementing contextDestroyed on the listener class, we should probably override the destroy method on the servlet itself.

I'd love to know what you think. Have a great weekend 👍

[weavejester]

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.

[dpiliouras]

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...

[dpiliouras]

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.

[weavejester]

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.

[dpiliouras]

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

[weavejester]

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

[dpiliouras]

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?

[weavejester]

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

[dpiliouras]

Thanks for the idea - see #219