[
https://issues.apache.org/jira/browse/CAMEL-25608?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Work on CAMEL-25608 started by shashank.
----------------------------------------
> camel-ical - the ical data format parses all exchanges with one
> CalendarBuilder, so two calendars unmarshalled at the same time get the
> events of each other
> ------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-25608
> URL: https://issues.apache.org/jira/browse/CAMEL-25608
> Project: Camel
> Issue Type: Bug
> Reporter: shashank
> Assignee: shashank
> Priority: Major
>
> {{ICalDataFormat}} creates one ical4j {{CalendarBuilder}} in {{doStart}}
> (line 61 at main a7169d7c6ab6) and {{unmarshal}} calls
> {{builder.build(stream)}} on it for every exchange (line 55), without a lock.
> A data format instance belongs to an {{unmarshal}} node of a route (or is a
> shared bean), so all threads that run the route use the same builder.
> A {{CalendarBuilder}} is not made for that (ical4j 4.4.0, read from the class
> files):
> * {{build(UnfoldingReader)}} is {{parser.parse(reader, contentHandler);
> return calendar;}}. {{contentHandler}} and {{calendar}} are fields of the
> builder.
> * the {{DefaultContentHandler}} keeps the calendar it is building in its
> fields ({{calendarProperties}}, {{calendarComponents}}, the stack of open
> components, the current property). {{startCalendar}} replaces the two lists,
> {{endComponent}} adds to them, {{endCalendar}} creates the {{Calendar}} from
> them and hands it to the builder, which stores it in the {{calendar}} field.
> So when two exchanges are in {{unmarshal}} at the same time:
> * the second {{startCalendar}} throws away the events the first exchange has
> parsed so far, and the remaining events of the first exchange are added to
> the calendar of the second one;
> * an exchange can return the {{calendar}} field after the other exchange has
> written it, that is the whole calendar of the other exchange;
> * when the two parsers are inside a component or a property at the same time,
> the open component of one exchange gets the properties or sub-components of
> the other, or the parse fails.
> No exception is needed for the wrong result: the route continues with a
> {{Calendar}} that has the events of another message. Any route that
> unmarshals ical with more than one thread is affected, with default options,
> for example
> {{from("jms:queue:invites?concurrentConsumers=5").unmarshal().ical(false)}},
> a {{seda}} endpoint with {{concurrentConsumers}}, a file consumer followed by
> {{threads()}}, or an HTTP consumer ({{platform-http}}, {{jetty}},
> {{undertow}}), which calls the route on the threads of the server. With
> stream caching (the default) the body is in memory when {{unmarshal}} runs,
> so the window is the parse itself; it gets longer with the size of the
> calendar.
> h3. Time zones of earlier messages (no concurrency needed)
> The shared builder also has one {{TimeZoneRegistryImpl}}, in which the
> content handler registers every {{VTIMEZONE}} it parses. Seen on main through
> the data format with ical4j 4.4.0, one route, one thread:
> * a calendar that uses {{TZID=Custom/A}} without a {{VTIMEZONE}} is read with
> the {{+03:00}} of an earlier calendar that defined {{Custom/A}}. As the first
> message the same calendar gets a local date-time without a zone.
> * a calendar that defines a {{TZID}} again with another offset ({{+0700}}) is
> read with the offset of the first definition ({{+03:00}}), also when it is
> 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; it was 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 ({{ZoneRulesProviderImpl$ZoneIdPool}}). With the shared builder they
> do 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()}}. So a route that receives calendars with a
> {{VTIMEZONE}} fails for them after 1500 such messages.
> {{marshal}} is not affected: {{CalendarOutputter}} keeps only the
> {{validating}} flag and the fold length.
> h3. Reproduction
> New
> {{ICalDataFormatConcurrentUnmarshalTest.overlappingUnmarshalsGetTheirOwnCalendar}}:
> a route {{direct:unmarshal -> unmarshal().ical(false)}}, two calendars with
> the events {{a1, a2}} and {{b1, b2}}. The stream of the first calendar gives
> the text up to the start of its second event and then waits (bounded) until
> the second calendar has been unmarshalled by another thread, so the two calls
> overlap in every run. The route has {{noStreamCaching()}} only so that the
> data format reads the stream of the test and the test can hold the first call
> inside {{unmarshal}}.
> On main: {{events of the first calendar ==> expected: <[a1, a2]> but was:
> <[b1, b2, a2]>}} in every run (three runs by the rerun setting of the build):
> the first exchange lost its first event and got the two events of the second
> exchange.
> With the fix both exchanges get their own two events.
> New {{ICalDataFormatTimeZoneTest.timeZoneOfAnEarlierCalendarIsNotUsed}}, 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. On main: {{the calendar that does not define the time
> zone ==> expected: <2026-01-01T12:00> but was:
> <2026-01-01T12:00+03:00[ical4j-local-...]>}} in every run. With the fix the
> second date is the local date-time.
> The redefined {{TZID}} and the zone id pool were checked with a throwaway
> test through the same route (not part of the patch).
> h3. Proposed fix
> {{unmarshal}} creates a {{CalendarBuilder}} per call ({{new
> CalendarBuilder().build(stream)}}), as the {{Calendars.load}} helpers of
> ical4j do; the field is removed. The constructor looks up the parser factory
> and creates a {{TimeZoneRegistryImpl}} (three empty maps; the standard time
> zones are in a static cache of ical4j), so the cost per message is small next
> to the parse. I did not measure it.
> Behaviour change, with a 4.24 upgrade guide entry: each calendar is read with
> its own time zone definitions only. With the fix, the date of a calendar that
> uses {{TZID=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}}); a
> redefined {{TZID}} is read with its own offset.
> {{getDateTimeStart().getDate()}} did not throw for the unknown {{TZID}}, with
> relaxed parsing on or off.
> Remaining limit of the zone id pool: with the fix 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 with the message above). A loop
> that did not keep the calendars and left the collector no 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). There is no test for the pool in the patch, because with the fix its
> result depends on GC timing.
> I did not keep one registry for all builders: {{TimeZoneRegistryImpl}} is not
> documented as thread-safe and one of its three maps ({{zoneIds}}) is a plain
> {{HashMap}}, so a shared registry could be the same kind of defect.
> camel-ical tests with the fix: 4 run, 0 failures (the two new tests and the
> two existing tests of {{ICalDataFormatTest}}).
> How it was found: a sweep for per-exchange state kept in fields of objects
> that are shared by concurrent exchanges, after CAMEL-25552. A small TLA+
> model of two unmarshals on one builder (actions: startCalendar, endComponent
> per event, endCalendar, return of the calendar field) violates "each exchange
> gets the events of its own stream" in a trace of 9 states (start b, start a,
> a adds its two events, b adds one, a ends and returns a1, a2, b1); with a
> builder per exchange the property holds for two and three exchanges. The
> model covers only the list of components and the result field, not the state
> inside a component or property. I then reproduced it with the real data
> format.
> Affected: main (a7169d7c6ab6; tested), camel-4.23.x, camel-4.22.x,
> camel-4.18.x and camel-4.14.x (the same three lines, 40, 55 and 61 of
> {{ICalDataFormat}}; read, not run). The time zone results are from main with
> ical4j 4.4.0 (also the version of camel-4.23.x); camel-4.22.x has ical4j
> 4.3.0, camel-4.18.x 4.2.3 and camel-4.14.x 4.1.1, where they were not run.
> The code is the one of CAMEL-6320 (2.12).
> Duplicate check (2026-10-13): JIRA text "CalendarBuilder" (none),
> "ICalDataFormat" (2: CAMEL-20375 a configurer NPE, CAMEL-13548 a Windows
> build failure), "ical thread" and "ical concurrent" (3 each, none about
> camel-ical), "camel-ical" (13, none about concurrency); GitHub pull requests
> "ical" in title: none about the builder.
> _Filed with Claude Code on behalf of allthingssecurity._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
