allthingssecurity opened a new pull request, #27795: URL: https://github.com/apache/camel/pull/27795
# Description [CAMEL-25608](https://issues.apache.org/jira/browse/CAMEL-25608) `ICalDataFormat` created one ical4j `CalendarBuilder` in `doStart` and `unmarshal` called `builder.build(stream)` on it for every exchange, without a lock. A `CalendarBuilder` keeps what it is parsing in its fields: `build` is `parser.parse(reader, contentHandler); return calendar;`, the content handler collects the properties and components of the calendar in its own fields, and `endCalendar` stores the result in the `calendar` field of the builder (ical4j 4.4.0, read from the class files). So two exchanges that were in `unmarshal` at the same time, on any route that runs with more than one thread (a JMS or seda consumer with `concurrentConsumers`, `threads()`, an HTTP consumer), mixed their calendars: the start of the second calendar dropped the events the first exchange had parsed so far, the remaining events of the first exchange were added to the calendar of the second one, or an exchange returned the calendar of the other. No exception is needed for that; the route continues with the events of another message. This change creates a `CalendarBuilder` per `unmarshal` call, as `Calendars.load` of ical4j does, and removes the field. `marshal` is unchanged: `CalendarOutputter` only keeps the `validating` flag and the fold length. The shared builder also has one time zone registry, where the parser registers every `VTIMEZONE` it reads. So the time zones of one message leaked into the next ones, without any concurrency. Seen on main through the data format (ical4j 4.4.0, one route, one thread): - a calendar that uses `TZID=Custom/A` without a `VTIMEZONE` was read with the `+03:00` of an earlier calendar that had defined `Custom/A`. The same calendar as the first message gets a local date-time without a zone. - a calendar that defines a `TZID` again with another offset (`+0700`) was read with the offset of the first definition (`+03:00`), also when it was sent a second time. `TimeZoneRegistryImpl.getZoneId` takes the first entry of a `HashMap` that has this `TZID` as value, so which definition wins is not specified; I saw the old one in each of the four reads I made. - every parsed `VTIMEZONE` takes one zone id from a pool of ical4j with 1500 ids. With the shared builder they did not come back: a loop that unmarshalled the same small calendar with one `VTIMEZONE` failed at the 1501st message with `ParserException: Error at line 12:Ran out of available zone IDs`, and so did the next message, also after a `System.gc()`. What changes for users (4.24 upgrade guide entry): each calendar is read with its own time zone definitions only. With the change, the date of the calendar that uses `Custom/A` without defining it is a `LocalDateTime` (`2026-01-01T12:00`) whatever was unmarshalled before; `TZID=Europe/Berlin` without a `VTIMEZONE` still resolves (`+01:00`); the redefined `TZID` is read with its own `+07:00`. I did not get an exception for the unknown `TZID` from `getDateTimeStart().getDate()`, with relaxed parsing on or off. What is left of the zone id pool: with the change the ids of a calendar are free again after the calendar has been garbage collected, not before. 1500 unmarshalled calendars with a `VTIMEZONE` that are all still referenced exhaust the pool (the 1501st unmarshal failed as above). A loop that did not keep the calendars and did not leave the collector time failed once after 2592 calendars and worked again after a `System.gc()`; that number depends on the collector and the heap. The pool size is the ical4j property `net.fortuna.ical4j.timezone.id.pool.size` (read from the class file, not tried). These loops are not in the PR as tests, because their result with the change depends on GC timing. Cost: the constructor of the builder looks up the parser factory and creates an empty time zone registry per message. I did not measure it; the standard time zones are in a static cache of ical4j. Tests: - New `ICalDataFormatConcurrentUnmarshalTest` in camel-ical: two calendars (`a1, a2` and `b1, b2`) are unmarshalled by the same route. The stream of the first one stops before its second event until the second calendar has been unmarshalled on another thread (bounded waits), so the calls overlap in every run. The route has `noStreamCaching()` only so that the data format reads the stream of the test itself. - Without the change: `events of the first calendar ==> expected: <[a1, a2]> but was: <[b1, b2, a2]>` (the first exchange lost its first event and got the two events of the second one). - New `ICalDataFormatTimeZoneTest` in camel-ical, one thread, default stream caching: a calendar that defines `Custom/A` (`+0300`) is unmarshalled, then a calendar that uses `TZID=Custom/A` without defining it. The second date has to be the local date-time. - Without the change: `the calendar that does not define the time zone ==> expected: <2026-01-01T12:00> but was: <2026-01-01T12:00+03:00[ical4j-local-...]>`. - With the change, camel-ical tests pass: 4 run, 0 failures (the two new tests and the two tests of `ICalDataFormatTest`). - The redefined `TZID` and the zone id pool were checked with a throwaway test through the same route on main and with the change; they have no test in the PR. Found with a small TLA+ model of two unmarshals on one builder (startCalendar, one step per event, endCalendar, return of the result field): "each exchange gets the events of its own stream" is violated in 9 states, and holds with a builder per exchange. The model covers the component list and the result field only. I then reproduced it with the real data format. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested camel-ical, including the formatter and import-sort plugins. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
