Clojurians Log v2

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.

@kenny: @ghadi and I reviewed this together and think the simplest solution is https://github.com/cognitect-labs/aws-api/commit/d1bfbef63e3d17d6f66fb88d38daf500ced27e9a

Yes, that does use core.async's thread pool, which is effectively onbounded, but threads are reclaimed after 60 seconds, and we think the risk is very low of this causing any real problems. If you disagree, please explain.

re: making fetch async, it already is, effectively. Even though a user implements fetch, it is only ever called via fetch-async. Make sense?

I think this will probably work for pretty much all use cases and don't have any arguments against using an unbounded pool at this time. Although, I think I would much prefer these threads to be named something more relevent to make for clearer thread dumps. Right, it is async but it is managed implicitly -- the user cannot manage the threads used to fetch credentials if desired. Returning a channel gives the api consumer control over how the credential fetching manages the threads. Perhaps no one would ever want that level of control. Not sure.

Tangentially, could we remove or change this sentence "See log for more details." in the anomaly message https://github.com/cognitect-labs/aws-api/blob/d1bfbef63e3d17d6f66fb88d38daf500ced27e9a/src/cognitect/aws/util.clj#L310?

{:cognitect.anomalies/category :cognitect.anomalies/fault
 :cognitect.anomalies/message (format "Unable to fetch %s. See log for more details." item)}

For that matter, why does fetch not support returning an anomaly and nil as failure conditions?

It would seem the anomaly response from the AssumeRole call would provide far better information than the current anomaly returned.

My guess is that was an effort to keep the implementation of https://github.com/cognitect-labs/aws-api/blob/d1bfbef63e3d17d6f66fb88d38daf500ced27e9a/src/cognitect/aws/credentials.clj#L132 simple, but I agree that anomalies would be better. The problem right now is fetch has "Return the credentials found by this provider, or nil." in the docstring, and changing implementations to return an anomaly instead risks breaking custom implementations that call fetch directly.

That said, an anomaly would definitely give us more control and more information!

Right. I'm thinking we may even want this in the near future to inform a user of our product that we are unable to authenticate using an IAM role they provided and giving them the AWS returned error message. Actually, I'm pretty sure returning an anomaly as is would already result in an anomaly getting returned haha

... result in the anomaly you returned getting returned to the caller. It just happens as an implementation detail though.

Within the aws-api implementation, yes. And we could always update fetch-async to decorate an anomaly with more info.

My concern is for the end user who wrote anything custom that dispatches on nil coming back directly from fetch.

The likelihood of that is probably very low, but I can't be sure.

Oh, I see. Oof.

CredentialsProvider2? 🙂

No joke!

Though that needs to be reviewed through a more holistic lens.

As in other things that could be included in this addition?