@stickfigure fyi do not close #326 yet, I'm planning on adding evict functionality eventually as well

@stickfigure fyi

Under a deadline, gonna take a deeper look at it this weekend.

@stickfigure gently reminder :)

I've taken a look at this patch and there's just something not quite right with it - information seems to be in the wrong place. Maybe we can restructure the data to accomplish your goal though.

You want all pending futures to be 'attached' to an Objectify instance, right? What if we just store them in the ObjectifyImpl and clear them on flush() instead of tracking them in a thread local? Would that work for your purposes?

That was my first instinct as well. Then I checked the code. PendingFutures might have been used without any Objectify instance present at all. If we would do that change it would break that functionality. For example "it is possible that CachingAsyncDatastoreService gets used - which utilizes this functionality - without any Objectify context".

Oh good point. Let me ponder this for a day.

@stickfigure Have you come to any conclusion?

Unfortunately I just fundamentally don't like this patch. The trigger future stuff was an ugly hack to begin with and this makes it waaaaay worse, to accommodate a use case that kinda seems like an abuse of the system to begin with.

That said, I'd like to accommodate you somehow. We just need to find another way, even if it means changing APIs. One possibility (that I haven't had the time to consider in depth) is that we can put the pending futures inside the AsyncCachingDatastoreService and give it a close() method. We'd need to permanently associate ACDS instances with Objectify instances rather than creating them ad hoc as they are right now, and it would require a minor version change (5.2) because the API changes (for folks using the cache directly).

What do you think? And are you interested in taking a stab at it? I am severely short of time right now.

That would limit the usability of TriggerFuture and PendingFutures. Right now they can be used without anything objectify related. I'm sure it wasn't an intended functionality, but it's not hard to imagine that people use it for every kind of async action that has a Future to ensure it finishes before the request does. So a complete revamp would break/remove this functionality.

On the other hand I see no issue with keeping a general TriggerFuture as it is now with the PendingFutures, and having ACDS-related trigger futures separated. So those would be closed when the Ofy instance/ACDS gets closed. The rest when the AsyncCacheFilter finishes.

What I can't see how it would be viable is that the ACDS wasn't a closeable until now. Ppl most likely used it like any other datastore service. So in order to make this work the ACDS should have an objectify-bound subclass that is closeable, and uses an internal list of futures to wait for, and the "original" general implementation, that would use the general pending future pool.

So based on this TBH I'm not convinced it would be cleaner without dropping functionality.

TriggerFuture/PendingFutures has never been part of the "public" API of Objectify and can be eliminated without remorse.

I don't think there are very many people using the Objectify cache outside of Objectify. And for those that are, I wouldn't feel terrible about adding a close() requirement and removing the "you must install the AsyncCacheFilter" requirement. It would require a version bump to 5.2 but that's not terrible.

At any rate, it's a solution that would accommodate your use case without adding excessive technical debt.