add support for :servlet-init and :servlet-destroy fns

[dpiliouras]

No description provided.

[weavejester]

Thanks for the PR. Can you amend the commit message to:

Add :servlet-init and :servlet-destroy options

[weavejester]

Should this if be around the -init and -destroy functions? We don't need the functions at all if the symbol is nil.

[dpiliouras]

I had the exact same thought, but it's not 100% clear to me whether it will work, since the init and destroy methods are now exposed by the generated class. If you're confident that gen-class doesn't care about not-exposing the listed as exposed methods, then I can have a go at your suggestion, otherwise I will have to test it against a real project.

[dpiliouras]

does this look right to you?

~(if init-sym
  `(defn ~'-init [this# cfg#]
      (~(generate-resolve init-sym))
      (. this# (~'superInit cfg#))))

[weavejester]

Probably should be:

~@(if init-sym
    `((defn ~'-init [this# cfg#]
        (~(generate-resolve init-sym))
        (. this# (~'superInit cfg#)))))

[dpiliouras]

Hi again, and apologies for the delay - last week was kind of hectic for me...

Ok, so feel free to check the new implementation. it does what you asked, and it fixes a little bug with the previous version (i.e. calling superInit after self). What follows is a little test straight on the REPL:

;; dummies
(def my-init #(println "INIT"))
(def my-destroy #(println "DESTROY"))

(let [servlet-ns  'foo.bar
      init-sym    nil
      destroy-sym nil]
;; SNIP - the syntax-quoted expression from lines 139 - 157 
 )

=>

(do
 (clojure.core/ns
  foo.bar
  (:gen-class :extends javax.servlet.http.HttpServlet :exposes-methods {init superInit, destroy superDestroy})
  (:import javax.servlet.http.HttpServlet))
 (def service-method)
 (clojure.core/defn -service
  [servlet__2073__auto__ request__2074__auto__ response__2075__auto__]
  (service-method servlet__2073__auto__ request__2074__auto__ response__2075__auto__))
 nil  ;; no init emitted
 nil) ;; no destroy emitted

Compare the above with the below (when init-sym and destroy-sym are non-nil)

(do
 (clojure.core/ns
  foo.bar
  (:gen-class :extends javax.servlet.http.HttpServlet :exposes-methods {init superInit, destroy superDestroy})
  (:import javax.servlet.http.HttpServlet))
 (def service-method)
 (clojure.core/defn -service
  [servlet__2299__auto__ request__2300__auto__ response__2301__auto__]
  (service-method servlet__2299__auto__ request__2300__auto__ response__2301__auto__))
 (clojure.core/defn -init
  [this__2296__auto__ cfg__2297__auto__]
  (. this__2296__auto__ (superInit cfg__2297__auto__))
  ((do (clojure.core/require (quote leiningen.ring.war)) (clojure.core/resolve (quote leiningen.ring.war/my-init)))))
 (clojure.core/defn -destroy
  [this__2298__auto__]
  ((do (clojure.core/require (quote leiningen.ring.war)) (clojure.core/resolve (quote leiningen.ring.war/my-destroy))))
  (. this__2298__auto__ superDestroy)))

So from the looks of it, we should be fine...I will follow this up with an confirmation after an actual run.

[dpiliouras]

BTW, if I use plain resolve instead of generate-resolve, i get something much nicer:

(do
 (clojure.core/ns
  foo.bar
  (:gen-class :extends javax.servlet.http.HttpServlet :exposes-methods {init superInit, destroy superDestroy})
  (:import javax.servlet.http.HttpServlet))
 (def service-method)
 (clojure.core/defn -service
  [servlet__2387__auto__ request__2388__auto__ response__2389__auto__]
  (service-method servlet__2387__auto__ request__2388__auto__ response__2389__auto__))
 (clojure.core/defn -init
  [this__2384__auto__ cfg__2385__auto__]
  (. this__2384__auto__ (superInit cfg__2385__auto__))
  (#'leiningen.ring.war/my-init)) ;; <=========
 (clojure.core/defn -destroy 
  [this__2386__auto__] 
   (#'leiningen.ring.war/my-destroy) ;; <=========
   (. this__2386__auto__ superDestroy)))

[dpiliouras]

I can confirm that the code works. On the topic of resolve VS generate-resolve, I think what we want is clojure.core/requiring-resolve, but that is kind of new (added on 1.10) so I'm not sure how you feel about it.

[dpiliouras]

Major update

It turns out that contextDestroyed works fine for our purposes (i.e. cleanup, persist files etc). The newly implemented Servlet::destroy also works, but I see stuff like this in the logs:

java.lang.UnsupportedOperationException: contextDestroyed (com.foo.bar.listener/-contextDestroyed not defined?)

Which not only is baffling (since contextDestroyed is always implemented), but also not sure if it's an actual problem, or caused by something else. In any case, if you ask me, I'd say we can merge this, but at the same time clearly document it as something that is typically not required.

Finally, what is your view on swapping thegenerate-resolve call for a plain resolve, or requiring-resolve (basically resolving the Var at compile time)?

TL;DR

There may be merit in merging this PR, as it adds functionality that someone might find useful, but not for the original reason I reported the issue #218, so that can be closed.