shashank created CAMEL-25608:
--------------------------------
Summary: 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
{{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)
