Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to ShimDandy 1.2.0 #314

Closed
wants to merge 1 commit into from
Closed

Conversation

tobias
Copy link
Contributor

@tobias tobias commented Oct 23, 2015

1.2.0 removes an avenue for leaking memory, and provides a close method
to call shutdown-agents on your behalf.

This should address #268.

1.2.0 removes an avenue for leaking memory, and provides a close method
to call `shutdown-agents` on your behalf.
@martinklepsch
Copy link
Member

👍 I'll give this a test drive later today and report back/merge. Thanks so much for your help on this @tobias!

@martinklepsch
Copy link
Member

@micha @alandipert @Deraen this requires a new binary release, did we come up with a consistent scheme for versioning those?

@Deraen
Copy link
Contributor

Deraen commented Oct 24, 2015

@martinklepsch In this case binary change is not breaking but will just fix problems? So I think version 2.4.0 would be good enough.

@Deraen
Copy link
Contributor

Deraen commented Oct 24, 2015

I could be wrong about breakage, the close method is new so calling it will break with old shimdandy.

@martinklepsch
Copy link
Member

@Deraen it's breaking as in old binary causes failure when ran with new libs:

        clojure.lang.ExceptionInfo: No matching field found: close for class org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl
    data: {:file
           "/var/folders/ss/4qg3hk1d4nv40phg1360ng5w0000gn/T/boot.user4226953933120525930.clj",
           :line 11}
java.lang.IllegalArgumentException: No matching field found: close for class org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl
                 ...
boot.pod/destroy-pod                           pod.clj: 470
     boot.user/-main  boot.user4226953933120525930.clj:   7
                 ...
   boot.user/eval243  boot.user4226953933120525930.clj:  11
                 ...
  boot.main/-main/fn                          main.clj: 161
     boot.main/-main                          main.clj: 161
                 ...
    boot.App.runBoot                          App.java: 242
       boot.App.main                          App.java: 356

@micha
Copy link
Contributor

micha commented Oct 24, 2015

I think we should just wrap with try/catch:

(when pod
  (try (.close pod)
    (catch java.lang.IllegalArgumentException _
      ;; compatibility with boot.App version 2.3.x
      (.invoke pod "clojure.core/shutdown-agents")
      (.. pod getClassLoader close))))

This will fall back to the previous behavior if an older version if shimdandy is present.

@tobias
Copy link
Contributor Author

tobias commented Oct 24, 2015

Ah, I neglected to consider how this would work with the app/lib split. It would also be safe to not call .close at all, and just continue to call shutdown-agents yourself, as that is all .close does currently.

I should also clarify that since the changes are all in shimdandy-impl, and the api changes were only additive (just .close), then a lib release alone will fix the memory leak, no need to rev the app.

@micha
Copy link
Contributor

micha commented Oct 25, 2015

Awesome! Thanks @tobias @martinklepsch @Deraen! 🍻

martinklepsch pushed a commit that referenced this pull request Oct 26, 2015
1.2.0 removes an avenue for leaking memory, and provides a close method
to call `shutdown-agents` on your behalf.
micha added a commit that referenced this pull request Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants