Add BufferRecyclerProvider configuration

[cowtowncoder]

No description provided.

[cowtowncoder]

Not 100% sure this is needed by life-cycle, but I think it may be.

[mariofusco]

In the most general case I believe that this method is necessary, even though for some implementations, like the ThreadLocal based one, it could be empty. If you turn this abstract class into an interface as I was suggesting, maybe you can also provide a default implementation for this method with an empty body?

[cowtowncoder]

Wasn't sure of naming for get/return etc, open to different names than acquire/release.

[mariofusco]

Naming wise I believe that what's wrong here is the name of the class. The fact that is a pool should be reflected in its name, so we should call it BufferRecyclerPool. Once you do this acquire and release will be the most natural choices, but of course I'm open to other suggestions.

[cowtowncoder]

Hmmmh. Ok. Not sure how I managed to miss this comment before merging.

I agree that if there's life-cycle, "provider" is not quite the right choice.
Not sure I like "pool" but maybe that'd be better...
Let me first merge things as they are and the consider renaming.

[cowtowncoder]

Was going back and forth between creating a main-level class vs nested here; mostly located here to keep close to defaultProvider() defined above.

[mariofusco]

I agree that it's better to keep all default implementations in one place and have them internal to the main level class. By the way we will also need another implementation not pooling anything but simply returning a new BufferRecycler every time that acquire is called. I believe that this "dummy" implementation should be also placed here.

[cowtowncoder]

I was wondering about "no-op" implementation but for now it did not seem necessary. But maybe I am wrong.

[cowtowncoder]

Oh. The reason -- maybe not great one -- was that the check for "do not recycle" is using a simple Feature, and by-passes provider/pool altogether.

Maybe using and passing no-op instance would make more sense, nonetheless, instead of using null provider/pool.

[cowtowncoder]

cc @mariofusco @pjfanning this would be building block for #1064: defined as separate PR so I can merge this scaffolding to master (for 3.0) first, since merging will get bit messy overall.

[mariofusco]

Making the BufferRecyclerProvider (or better Pool imo) pluggable is actually a great decision, also because it will allow users to provide their own implementation, possibly based on an already existing pooling library, without forcing jackson-core to also directly depend on that library. Why didn't I think about it? :)

My only outstanding doubt is about the IOContext constructor and life-cycle. The IOContext is actually the only user and owner of the BufferRecycler, so it's evident that their 2 life-cycles are related and the second should be returned to the pool only when the first is closed. So my doubts are:

• Is it reasonable to allow to create an IOContext without going through the JsonFactory? At the moment we have many test cases doing this, but I don't understand if this is a valid use case and for which situation.
• If so should there exist a constructor of the IOContext that doesn't take the BufferRecycler as an argument? I introduced it in my other pull request because I wanted to make it clear that the first is the owner of the second and also controls its life-cycle. In general I still believe that this was a good idea (if a resource has only one exclusive owner it should be possibly created by that owner to fully encapsulate its life-cycle in the owner implementation), but this specific situation maybe doesn't fit that general good practice.

Once you will have clarified this doubts, and eventually addressed my other comments in the code, please feel free to merge this pull request asap and I will take care to rebase mine against it and rework it accordingly.

[pjfanning]

I think the IOContext should normally be tried to a JsonFactory and that there is an argument that the a buffer recycler could be reused by all IOContexts associated with a JsonFactory instance.

If there is test code that creates IOContexts directly, then there might be an argument for us to change those to use JsonFactories.

@cowtowncoder can clarify if IOContexts need to creatable without Json Factories.

[cowtowncoder]

@mariofusco @pjfanning You are correct: IOContext should never be created by anything other than JsonFactory (and it subtypes). Some tests do create it to simplify set-up but that should not be done by non-test code.
Ideally these test dependencies would be reduced to minimum or removed; but if not, perhaps IOContext should have specific test support method(s) (factory method probably).

EDIT: Ok actually there is at least one alternate/secondary owner for BufferRecyclers: ObjectMapper methods writeValueAsBytes() (and ...AsString()).
They do not use IOContext, just recycler. So perhaps supporting would be easy enough.

This leads to another thing that is bit open: need for BufferRecyclerProvider.releaseBufferRecycler() (or lack thereof).
To use that, IOContext would need to be passed both Provider and Recycler -- but it might be simpler and safer to make Provider pass itself (or just Function?) to Recycler, to be called when closing.

I think I will remove this method before merging; it (or similar) can easily be added when scaffolding is ready; for now only acquire method is really needed.

[cowtowncoder]

Merged: looking at code I do think that BufferRecycler itself should probably have release() (or similar) that will release itself to provider (pool) that created it. Not needed for ThreadLocal-based provider, fwtw; but for others.
Doing this would require adding BufferRecycler constructor that takes BufferRecyclerProvider and retains reference to it.

WDYT?

[cowtowncoder]

Quick note: I will do another PR with at least following changes:

1. Rename Provider -> Pool
2. Add "release" method back -- I finally grokked why this is almost certainly needed (I think we'll pass Pool instance through BufferRecycler, so no custom sub-classes)
3. Add no-op implementation

[cowtowncoder]

Did #1085, merged it. So I think this is closer to working baseline.

[mariofusco]

Merged: looking at code I do think that BufferRecycler itself should probably have release() (or similar) that will release itself to provider (pool) that created it. Not needed for ThreadLocal-based provider, fwtw; but for others. Doing this would require adding BufferRecycler constructor that takes BufferRecyclerProvider and retains reference to it.

WDYT?

Agreed, and I did something similar in my pull request. Actually there also BufferRecycler implements AutoCloseable and its close() method behaves as a release() returning the instance to the pool. I'm no longer sure if what I did is semantically correct because there you're not really closing the instance, but only making it available to the pool again, so yes, I guess that I will use release() also in my pull request.