search

clj-commons / manifold

A compatibility layer for event-driven abstractions
1.02k stars 106 forks source link

Extend functionality of the thread-factory builder #161

Closed kachayev closed 5 years ago

kachayev commented 5 years ago

There're 2 different extensions here:

  1. daemon? configuration, backward compatible with old usage patterns. We need this to use executor/thread-factory in Aleph project (as we need both daemon and non-daemon threads there).

  2. Set context class loader for each new thread. More info here. That's actually done in a kinda "defensive" style. Those threads would have context class loader set to a parent thread loader... in most cases. So we're kinda safe here using RT.baseLoader in Aleph that falls back to a current thread class loader at least with the default configuration and with runtime class loading path the ignores "non-Clojure" class loaders. I prefer to left the assignment here to workaround different behavior of security manager that might prevent referencing to the parent class loader. We still can have a problem with *use-context-classloader* set to false, but that's something you have to do manually. And I hope in such a case the user understands the reasons and consequences.

kachayev commented 5 years ago

P.S. After this is merged, I'll switch thread factories in Aleph.

ztellman commented 5 years ago

I was trying to figure out if it made more sense to capture the classloader in newThread or when the thread factory is instantiated, but I guess it doesn't make much difference. Merging now.

ztellman commented 5 years ago

Actually, on further reflection, it's very possible that execute will get called on the thread pool on a random thread without the proper classloader, but exceedingly unlikely that thread-factory will get invoked in such a thread. I'm pretty sure we should just close over it, but you've thought about this problem more than I have, so let me know if I'm missing something.

kachayev commented 5 years ago

on the thread pool on a random thread without the proper classloader

The idea here to use any loader that was used to load thread-factory function by itself. In most cases, this loader should be good enough to load any Clojure-related code. You still can shoot in your foot, but I don't think that's easy to do unintentionally.

We can move capturing of the curr-loader out of the newThread method, I think. At least I couldn't come up with any scenario when that doesn't work.