Clojurians Log v2

@kenny seems like a reasonable near term approach.

When you have free time, curious about your thoughts on the real solution.

Implicit creds fetch to a separate pool?

Separate pool for creds fetch is a half solution too. I suppose you could have arbitrarily nested cred fetching that needs to run.

Returning a channel would probably fix this entirely, but I’m assuming there’s a reason it’s run in a thread.

It does return a channel - we're just putting on that channel in a thread from that pool. Making that async/thread is my first instinct. So it's obviously wrong 😉 If you have an opinion about that, I'm all ears. Also want to hear from @ghadi.

I deployed workaround with async-fetch-pool set to 10 yesterday evening. The service has been healthy since. For those looking for a workaround, I can say with high certainty that this will fix it. Wrapping in async/thread as @dchelimsky says sounds like another potential solution. I do believe that results in an unbounded number of potential threads to be created. In this case, that should always be small I think.

It's bound by core.async's threadpool size, which defaults to 8, but you can configure it via the clojure.core.async.pool-size system property: https://github.com/clojure/core.async/blob/1c5efa72ebf65cd23315311d2bc3d890b780696e/src/main/clojure/clojure/core/async/impl/exec/threadpool.clj

I don't think async/thread is bound by that. That's for the go blocks.

thread calls thread-call which uses thread-macro-executor, defined as a CachedThreadPool https://github.com/clojure/core.async/blob/1c5efa72ebf65cd23315311d2bc3d890b780696e/src/main/clojure/clojure/core/async.clj#L471.

The maximumPoolSize for core.async's call to newCachedThreadPool is Integer.MAX_VALUE.

So it is bounded 😉. But yeah, we'll need to constrain that a bit more.

I think CredentialsProvider fetch needs to return a channel. That would move the problem to the user.

Also, I think that is effectively the same as unbounded. Fairly certain bad things will happen as you approach the max int value.

Oh yeah, I was making a joke about it being bounded.

Ohh, got it 🙂

re: returning a channel from fetch, that would be a breaking change so we won't do that, but we could add fetch-async to the CredentialsProvider and return a channel from that (and use that in our implementations).

It would indeed. Deprecating the current method seems like a good idea since it can result in a deadlock. Deadlocks are one of the worst problems to hit.

I'll have some time to look at this and talk some things over with other contributors tomorrow so we should have at least a direction. I'll follow up on the issue at that point.