fix external pool pluggability

[mariofusco]

Trying to plug external pools from Quarkus I found that I made a terrible mistake so that external pools are never registered on the pooled BufferRecycler and then it never gets released to the pool. This pull request is intended to fix this bug.

@cowtowncoder @pjfanning Sorry about this, but please try to give it the highest possible priority. Let me know if you have any question.

/cc @franz1981

[cowtowncoder]

Any chance we could verify life-cycle without directly calling internal method(s)?

[cowtowncoder]

@mariofusco Ok, just to make sure I understand: your concern is that expecting external implementations to know not just to acquire (possibly) pooled BufferRecycler, but also call withPool() is error-prone? And so instead, should extract part of acquiring and handle linking separately by default?
If so, makes sense.

I think I'll change the naming/structure a bit though: sort of reversing naming you suggest. So externally called (public) method remains what it was, and we'll just introduce another protected method... oh shoot. Interfaces don't allow protected. This is actually one reason to prefer abstract classes.
Well, I'll leave underscore-named public default method as the thing for pool implementations to (have to) implement.

[cowtowncoder]

@mariofusco Apologies for significant rewrite here; I realized I'd want to hide the new internal method, expose existing Acquire-method, and things went bit further from there.
Just to make sure I did not completely misunderstand intent here I won't merge it without you having a look -- if this looks ok please LMK ASAP (well, if it does NOT make sense also LMK :) ).

[mariofusco]

@mariofusco Ok, just to make sure I understand: your concern is that expecting external implementations to know not just to acquire (possibly) pooled BufferRecycler, but also call withPool() is error-prone? And so instead, should extract part of acquiring and handle linking separately by default? If so, makes sense.

That's entirely correct. Also consider that the withPool method is package private and I would like to keep it as it is. It is an internal implementation details and users should know anything about it.

[mariofusco]

@mariofusco Apologies for significant rewrite here; I realized I'd want to hide the new internal method, expose existing Acquire-method, and things went bit further from there. Just to make sure I did not completely misunderstand intent here I won't merge it without you having a look -- if this looks ok please LMK ASAP (well, if it does NOT make sense also LMK :) ).

Hi @cowtowncoder, you understood correctly the reasons why I sent this pull request, but in all honesty there are 2 things that I don't like with your changes.

The first one (ugly but I could live with this) is that now you're asking to external pool implementors to extend an abstract class instead of implementing an interface. This is a bit limiting due to Java single inheritance (implementors may want for instance to extend an existing data structure, maybe taken from an external library and just implement the BufferRecyclerPool interface) and in my opinion not necessary.

The second one (even worse than the other) is the asymmetry imposed to a pool implementor that when extending your abstract class now has to implement 2 methods: _internalAcquire AND releaseBufferRecycler. This is extremely confusing.

In essence my point of view is the following: designing an API sometimes requires some trade-off and when you need to do so it is clearly better to accept some inconvenience to the private part of the API, leaving a nicer look to the public part. I believe that in this case you mistakenly swap those private and public parts. In this situation the public part (what the user is supposed to do) is developing a new implementation of the pool, while the private part (what jackson-core internally does) is using the pool eventually provided by the user. The changes that you're proposing improve the private part making the public one much worse.

Please let me know if my analysis makes sense.

[mariofusco]

@cowtowncoder This is an alternative solution. Let me know which one you prefer.

[cowtowncoder]

Hmmh. Interesting suggestion wrt private vs public part. To me, public part is the thing called from external entity, in this case jackson-core components (JsonFactory); internal part is what implementations provide.
But I can see why the reverse is a valid view point.

Abstract class vs interface is trickier: I really dislike full public-ness of interfaces. But it is true that interfaces are easier for implementors.

Let me think about this for a moment.

[cowtowncoder]

Ok. I prefer this approach.
So then it's just the question of how much to roll back here, choose naming. Here's my suggestion:

1. Change BufferRecyclerPool back to interface as suggested
2. Expose 3 public methods as earlier, but use "regular naming" for both acquire-related methods. Leave 2 unimplemented ("internal" acquire that does NOT link recycler to pool; "release" that recycler calls)

Possible naming for 2 acquire-related methods:

1. acquireBufferRecycler() that does not do linking -- one fully abstract (no default impl)
2. acquireBufferRecyclerAndLink() that by default calls acquireBufferRecycler() and then withPool() -- overridable but only really overridden if no recycling is done.

Alternatively we could also always use default impl for (2): even if no recycling occurs it's fine to call "release"-method as pool will just ignore it.

If I have time today I will do changes above.

[mariofusco]

What you're suggesting is not that far from my original implementation, so ok for me. I will wait for your changes and give one final review. Let me know if I could do something else.

[cowtowncoder]

@mariofusco Thank you & sorry for the mess. I should have discussed my idea before making changes here.

[cowtowncoder]

@mariofusco Ok, here you go. Please make any changes you think are needed, let me know once this is ready to be merged from your perspective.

[mariofusco]

This looks almost identical to my original implementation with the only (relevant and nice) difference that the method that I called _internalAcquireBufferRecycler is now rightly considered part of the public API and called acquireAndLinkBufferRecycler which is indeed a clearer name.

I'm happy with this so please feel free to merge this pull request asap. I will close the other pull request implementing my alternative solution.

[cowtowncoder]

Done; merged into 2.16, forward to master (3.0); all tests passing.

Also realized that 2 of dataformats have ThreadLocal based recycling beyond BufferRecycler -- Smile and Avro.
Will need to work on those as well.

[pjfanning]

Also realized that 2 of dataformats have ThreadLocal based recycling beyond BufferRecycler -- Smile and Avro. Will need to work on those as well.

And https://github.com/FasterXML/jackson-jaxrs-providers/blob/f8dd37d5a263886b476c86f60bc6970ed6de99fc/base/src/main/java/com/fasterxml/jackson/jaxrs/cfg/ObjectWriterInjector.java#L16 (plus its jakarta fork)

And yaml format too - https://github.com/search?q=repo%3AFasterXML%2Fjackson-dataformats-text+ThreadLocal+language%3AJava&type=code&l=Java