Servlet instances leak custodian-managed resources

[kimmyg]

When servlet instances are flushed (selectively or not), the only action taken is that servlet instance references are discarded. In particular, resources managed by the servlet custodian are not shut down but are instead managed (directly) by the superordinate custodian.

Should the servlet custodian be shut down when the instance is flushed?

For my use case (spawning a service thread), it looks like I can get by with a will, but shutting down the custodian seems like a more “natural” general policy. (And maybe it is, but is also a breaking change. Is that the case?)

[jeapostrophe]

Are you referring to how make-cached-url->servlet creates a cache-table mapping paths to servlet structures and then you can use the cache-table-clear! callback to clear the cache and reload them?

I believe it was designed under the assumption that an unreachable custodian is shutdown automatically, but it seems that you're saying that is not true? Do you have a problem case? (I am ever so slightly skeptical because this path is only used when you (a) use the deprecated command-line tool or (b) use serve/servlet and then still load servlets from disk, rather than providing them as a function argument.

Supposing this is the problem, I think the solution is to change cache-table-clear! to take a function argument that it will call on every member of the cache table. The function should be (lambda (s) (custodian-shutdown-all (servlet-custodian s))) and it can default to void to stay backwards compatible.

[kimmyg]

Yes, that's what I'm referring to. I should have been more specific.

I don't think unreachable custodians are shut down. The docs vaguely suggest not and I can't see that behavior in tests.

I'm using make-cached-url->servlet to let me load/unload servlets from disk in a dispatcher run by serve. Unloading a servlet doesn't kill long-running threads that it started, so, if I reload the servlet, I get duplicate threads.

If you're skeptical this should be a problem, then I'm more interested in a better design. Otherwise, I can submit a PR with the change.

[jeapostrophe]

Okay, I think we should do the change. Maybe we can get confirmation from @mflatt that unreachable custodians don't get shutdown.

[mflatt]

Custodians are themselves custodian-managed objects, so they don't really become unreachable and they aren't shut down implicitly.

[jeapostrophe]

Ah, that makes sense!

…

On Mon, Dec 11, 2017 at 2:15 PM, Matthew Flatt ***@***.***> wrote: Custodians are themselves custodian-managed objects, so they don't really become unreachable and they aren't shut down implicitly. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOM-lnn8mjnd95hsbAcgli-MT3WrQhoks5s_X9ggaJpZM4Q86mL> .

-- -=[ Jay McCarthy http://jeapostrophe.github.io ]=- -=[ Associate Professor PLT @ CS @ UMass Lowell ]=- -=[ Moses 1:33: And worlds without number have I created; ]=-

[kimmyg]

The docs say that custodians only weakly reference the objects they manage and only weakly reference their subordinate custodians. What am I missing?

[mflatt]

Ok, but the docs also say that a custodian takes over a disappearing subordinate custodian's objects.

The weak reference may actually be implemented by ordered finalization, and custodian-managed-list may expose the difference, so I'll have to revisit those docs.

[jessealama]

@kimmyg are you satisfied with the discussion? Do you think the docs should be changed? (To put the question differently: do you consider this issue closed?)

[kimmyg]

I think the docs should warn that the default behavior can leak resources. Specifically, I would welcome a change in the documentation for make-cached-url->servlet from

The second optional argument is a procedure which is invoked on each cached value before it is flushed, which can be used to finalize servlet resources. It defaults to void.

to

The second optional argument is a procedure which is invoked on each cached value before it is flushed, which can be used to finalize servlet resources. Beware that the default value void performs no finalization. In particular, it does not shut down the servlet's custodian, allowing the servlet's custodian-managed resources (such as threads) to persist.

(or similar).

With such a documentation change, I would consider this issue closed. If my particular change is acceptable, I'm happy to make it in a pull request tomorrow.

[jessealama]

That looks like a sensible addition to the docs to me.

@mflatt & @jeapostrophe would you agree?

[jeapostrophe]

Agree