davsclaus commented on PR #28651: URL: https://github.com/apache/camel/pull/28651#issuecomment-6488937377
Review note on the lookup cache (checked every place that mutates `converters`: `addConverter`, `addTypeConverter`, `removeTypeConverter`, the convert-path `putConverter` calls, and the version-counter race handling; the invalidation looks correct to me). One design concern: the new `lookups` map is unbounded, its keys hold strong `Class` references, and misses are cached as well. `lookup(toType, fromType)` is not only called by `EnumTypeConverter` with `(enum, String)`; `BeanInfo`, `ValidatingProcessor`, `XmlSourceHandlerFactoryImpl` and `PQCProducer` call it with `body.getClass()` / parameter types. With dynamically generated body classes (Groovy, proxies, hot reload, dev mode) the cache would grow and pin class loaders. The `converters` map already records `MISS_CONVERTER` per pair on the convert path, so this is not a new class of problem, but it widens it. Options: bound the cache, or only cache the enum path (for example, cache inside `EnumTypeConverter` keyed by the enum type). If keeping the same exposure as `converters` is acceptable, please say so in the PR description. There is also an unresolved bot thread on this PR to look at. _Claude Code on behalf of davsclaus_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
