On Sat, Oct 18, 2025, 17:44 Piotr P. Karwasz <[email protected]> wrote:
> > Hi Gary, > > On 17.10.2025 16:49, [email protected] wrote: > > - static final class ScratchBufferHolder { > > + static final class ScratchBytes implements AutoCloseable { > > > > /** > > - * Holder for internal byte array buffer. > > + * Wraps an internal byte array. > > + * > > + * [0] boolean in use. > > + * [1] byte[] buffer. > > */ > > - private static final ThreadLocal<Object[]> > SCRATCH_BYTE_BUFFER_HOLDER = ThreadLocal.withInitial(() -> new Object[] { > false, byteArray() }); > > - > > - /** > > - * Holder for internal char array buffer. > > - */ > > - private static final ThreadLocal<Object[]> > SCRATCH_CHAR_BUFFER_HOLDER = ThreadLocal.withInitial(() -> new Object[] { > false, charArray() }); > > - > > + private static final ThreadLocal<Object[]> LOCAL = > ThreadLocal.withInitial(() -> new Object[] { false, new > ScratchBytes(byteArray()) }); > > > > /** > > * Gets the internal byte array buffer. > > * > > * @return the internal byte array buffer. > > */ > > - static byte[] getScratchByteArray() { > > - final Object[] holder = SCRATCH_BYTE_BUFFER_HOLDER.get(); > > + static ScratchBytes get() { > > + final Object[] holder = LOCAL.get(); > > // If already in use, return a new array > > if ((boolean) holder[0]) { > > - return byteArray(); > > + return new ScratchBytes(byteArray()); > > } > > holder[0] = true; > > - return (byte[]) holder[1]; > > + return (ScratchBytes) holder[1]; > > + } > > > I like the idea of splitting ScratchBufferHolder into ScratchBytes and > ScratchChars, but storing custom classes inside a ThreadLocal can cause > classloader memory leaks in application servers during redeployments. > > I took that into account in PR #801 when introducing > ScratchBufferHolder, which is why I intentionally did not use > AutoCloseable at the time. > > I followed up on your change and submitted PR #804 [2], which: > - Removes the classloader leak risk, > - Preserves your refactoring and usability improvements. > > However, I think we should discuss whether AutoCloseable is worth keeping: > > Option 1: Merge PR #804 > Hi @ppkarwasz Good catch. Merged. Too bad there's no way to unit test this rentention use case. This likely warrants a review of other Commons components for thread local usage. Ty! Gary - Pros: Preserves your usability improvements > - Cons: Adds a second ThreadLocal lookup per access > Option 2: Revert the commit introducing AutoCloseable > - Pros: Restores original single lookup performance > - Cons: Slightly more verbose usage > > Given that ScratchBytes/ScratchChars are used on hot paths, this > trade-off matters. > > I would also like to kindly ask that changes related to a PR are > discussed in the PR rather than pushed directly to master afterward. I > know we differ on CTR vs. RTC, but keeping changes self-contained helps > avoid a chain of post-merge fixes and makes review easier for everyone. > The goal of a PR is to prevent this kind of back-and-forth after merging. > > Thanks, > Piotr > > References: > [1] https://github.com/apache/commons-io/pull/801 > [2] https://github.com/apache/commons-io/pull/804 > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
