https://issues.apache.org/bugzilla/show_bug.cgi?id=55317
--- Comment #19 from Nick Williams <nicho...@nicholaswilliams.net> ---
(In reply to Jeremy Boynes from comment #18)
> 1) CopyOnWriteArrayList would simplify managing added transformers and is
> intended for this use-case.
How have I never heard of this class before? Certainly, mutation operations
will definitely vastly outnumber traversal operations. Perhaps
CopyOnWriteArrayList is the way to go here, instead of any locking. Mark?
Thoughts?
> 2) I would not try to de-dupe the list at all. There's no contract around
> equals() for transformers and this will be different from how JRE's
> Instrumentation manages them.
You're correct, there's no contract for equals in ClassFileTransformer. By
default a "duplicate" will be if the transformers are the exact same instance
(==). Only if the person who implemented the transformer overrides
Object#equals will a different behavior occur. Arguably, if the person
overrides Object#equals, they likely did so to specify a different definition
of "duplicate." I disagree that we shouldn't try to keep duplicates out of the
list. I /would/ be open to saying that we should specifically look for
duplicate instances (==) and not rely on Object#equals, in which case we need
to go back to iterating over the list instead of using List#contains.
> 3) You're applying the transformers in findResourceInternal() which, I
> think, means getResource() will return transformed data. The transform
> should be moved into findClass() so only defined classes are transformed.
#findResourceInternal definitely feels like the right place for this. Re: "so
only defined classes are transformed," how I've written it means only classes
are transformed because the transformers are only applied if isClassResource is
true. I don't see a problem here, but maybe I don't understand what you're
saying.
> 4) In the messages, we typically wrap [] around the substituted values i.e.
> [{0}]
I would challenge "typically". Looks like about 50% of the time to me. If I'm
being told that only [{0}] is right, I'll change it. But there is /plenty/ of
code that isn't "correct" if this is the case. How are contributors to know
which to use? They can't.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
