You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "henrik242 (via GitHub)" <gi...@apache.org> on 2026/09/04 08:38:57 UTC
[PR] Camel 24626 master leadership followups [camel]
henrik242 opened a new pull request, #26112:
URL: https://github.com/apache/camel/pull/26112
Fixes [CAMEL-24626](https://issues.apache.org/jira/browse/CAMEL-24626).
Three follow-ups to #26028, listed as known pre-existing issues in that PR's description. None of
them was introduced or fixed there.
## 1. A cancelled `BackgroundTask` stays in the `TaskManagerRegistry` (camel-support)
A task scheduled through `BackgroundTask.schedule()` adds itself to the `TaskManagerRegistry` from
its first run, and only a run of the task removes it again. A caller that cancels the `Future`
returned by `schedule()` therefore leaves the entry behind for the life of the `CamelContext`: the
task keeps being listed as an internal task, and it keeps its container reachable.
`BackgroundTask.cancel(boolean)` now unschedules the task, releases the latch so no queued run does
any work, marks the task `Inactive` and removes it from the registry. A run that starts while a
cancel is landing undoes its own registration, so the two cannot race a stale entry back in.
`schedule()` is used in exactly two places, `camel-sjms` and `camel-master`, and both now cancel
through the task. The other thirteen `Tasks.backgroundTask()` users go through the blocking `run()`,
which already deregisters on every exit path.
## 2. Lock inversion between the consumer and the cluster view (camel-master)
`MasterConsumer` guarded its leadership state with the `BaseService` lock:
- `doStop` holds that lock and then needs the write lock of the view, through
`CamelClusterView.removeEventListener`.
- `AbstractCamelClusterView` dispatches events while holding its own read lock, and the listener of
the consumer then needs the `BaseService` lock.
The unlocked `isRunAllowed()` fast path added in #26028 covers the common case, but a leadership
event that passes that check just before a stop acquires the lock still closes the two orders into a
deadlock.
The leadership state and the pending start task now have a lock of their own, which `doStop`
releases before it touches the view. Nothing that holds it ever waits for the view, so the cycle is
gone rather than narrowed. It also decouples leadership handling from the service lifecycle: a
leadership event and a start attempt no longer wait for whatever lifecycle operation is in progress.
One correction to the description of #26028, which claimed `doStop` waits for the leader pool to
terminate while an in-flight attempt waits for the lock. It does not:
`ExecutorServiceManager.shutdown()` passes an await timeout of 0, so that path was unnecessary
coupling rather than a deadlock. The code comment says what actually happens.
## 3. Exhausted start attempts are not documented (camel-master)
`backOffMaxAttempts` defaults to 10 attempts, `backOffDelay` apart (5000 millis). A node that uses
up its attempts keeps the leadership and consumes nothing until the leadership changes again. That
is the documented intent of the option, and `backOffMaxAttempts=0` already retries for as long as
the node is the leader, because the budget builder ignores non-positive values and keeps its
unlimited default. Neither the consequence nor the escape hatch was written down, so both are now in
the component documentation.
## Testing
Two tests in `MasterConsumerLeadershipTest`, both checked against unpatched `main` and failing
there:
- `testCancellingAPendingStartRemovesTheTaskFromTheRegistry` keeps a start task retrying, loses the
leadership, and asserts the task leaves the registry. Before the change it stays.
- `testEventDispatchIsNotBlockedByALifecycleOperation` holds the service lock through a suspend that
blocks inside the delegate, then asserts a leadership event is still dispatched. Before the change
the dispatch waits for the lock, which is the wait that deadlocks against the view.
Two tests in `BackgroundTaskTest` cover cancelling a task that is running and cancelling one before
its first run: unscheduled, deregistered, `Inactive`, and no further attempt.
Green locally: camel-support (119), camel-master (30), the camel-core task tests (34), and
`-Psourcecheck` on both changed main modules. The camel-sjms main sources compile, but its tests
need an Artemis test-infra artifact that is not installed locally, so CI has to cover that call site.
## Notes for the reviewer
- The description of the `backOffMaxAttempts` option itself is deliberately unchanged. It feeds
three generated mirrors (the component json, the catalog json and `MasterComponentBuilderFactory`),
and the catalog and componentdsl ones cannot be regenerated without a full build, so a hand-edited
version would fail the uncommitted-changes check. Worth a small follow-up from someone with a full
build, since that description is what tooling and IDE completion show.
- Both bugs are present on `camel-4.22.x` as well: the registry leak through camel-sjms predates
this work, and the lock inversion came along with the backport of CAMEL-24583. Happy to open a
backport PR if you want it there.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5635883692
<!-- camel-backport-bot -->
### đ Backport Bot
**Conflicts (đ¤ agent dispatched to resolve):**
- â ī¸ `camel-4.18.x` â Cherry-pick conflict in: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/master-component.adoc, components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java, components/camel-master/src/test/java/org/apache/camel/component/master/MasterConsumerLeadershipTest.java, core/camel-core/src/test/java/org/apache/camel/support/task/task/BackgroundTaskTest.java, core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java
âšī¸ If you push additional commits after `/port`, use `/port` again to update the port PRs.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5553181133
@henrik242 take a look at the comments from @gnodet
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3947932954
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,53 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
Review Comment:
Agreed, the order holds.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5551664676
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `catalog/camel-catalog`
- `components/camel-master`
- `components/camel-sjms`
- `core/camel-core`
- `core/camel-support`
> :information_source: Dependent modules were not tested because the total number of affected modules exceeded the threshold (50). Use the `test-dependents` label to force testing all dependents.
---
<details><summary>:microscope: Scalpel shadow comparison â Scalpel: 563 tested, 26 compile-only â current: 562 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **589 affected modules** (current approach: 562).
<details><summary>:warning: Modules only in Scalpel (27)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
**Skip-tests mode would test 563 modules** (5 direct + 558 downstream), **skip tests for 26** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (563)</summary>
- `archetypes`
- `camel-a2a`
- `camel-activemq`
- `camel-activemq6`
- `camel-ai-observability`
- `camel-ai-observability-api`
- `camel-ai-parent`
- `camel-ai-resource`
- `camel-ai-tool`
- `camel-alibaba-common`
- `camel-alibaba-eventbridge`
- `camel-alibaba-fc`
- `camel-alibaba-kms`
- `camel-alibaba-mns`
- `camel-alibaba-oss`
- `camel-alibaba-ots`
- `camel-alibaba-parent`
- `camel-alibaba-sls`
- `camel-alibaba-sms`
- `camel-amqp`
- `camel-api-component-maven-plugin`
- `camel-arangodb`
- `camel-archetype-api-component`
- `camel-archetype-component`
- `camel-archetype-dataformat`
- `camel-archetype-java`
- `camel-archetype-main`
- `camel-as2`
- `camel-as2-api`
- `camel-as2-parent`
- `camel-asn1`
- `camel-asterisk`
- `camel-atmosphere-websocket`
- `camel-atom`
- `camel-attachments`
- `camel-avro`
- `camel-avro-rpc`
- `camel-avro-rpc-jetty`
- `camel-avro-rpc-parent`
- `camel-avro-rpc-spi`
- `camel-aws-bedrock`
- `camel-aws-cloudtrail`
- `camel-aws-common`
- `camel-aws-config`
- `camel-aws-parameter-store`
- `camel-aws-parent`
- `camel-aws-secrets-manager`
- `camel-aws-security-hub`
- `camel-aws2-athena`
- `camel-aws2-comprehend`
- `camel-aws2-cw`
- `camel-aws2-ddb`
- `camel-aws2-ec2`
- `camel-aws2-ecs`
- `camel-aws2-eks`
- `camel-aws2-eventbridge`
- `camel-aws2-iam`
- `camel-aws2-kinesis`
- `camel-aws2-kms`
- `camel-aws2-lambda`
- `camel-aws2-mq`
- `camel-aws2-msk`
- `camel-aws2-polly`
- `camel-aws2-redshift`
- `camel-aws2-rekognition`
- `camel-aws2-s3`
- `camel-aws2-s3-vectors`
- `camel-aws2-ses`
- `camel-aws2-sns`
- `camel-aws2-sqs`
- `camel-aws2-step-functions`
- `camel-aws2-sts`
- `camel-aws2-textract`
- `camel-aws2-timestream`
- `camel-aws2-transcribe`
- `camel-aws2-translate`
- `camel-azure-common`
- `camel-azure-cosmosdb`
- `camel-azure-eventgrid`
- `camel-azure-eventhubs`
- `camel-azure-files`
- `camel-azure-functions`
- `camel-azure-key-vault`
- `camel-azure-parent`
- `camel-azure-schema-registry`
- `camel-azure-servicebus`
- `camel-azure-storage-blob`
- `camel-azure-storage-datalake`
- `camel-azure-storage-queue`
- `camel-barcode`
- `camel-base`
- `camel-base-engine`
- `camel-base64`
- `camel-bean`
- `camel-bean-validator`
- `camel-beanio`
- `camel-bindy`
- `camel-bonita`
- `camel-box`
- `camel-box-api`
- `camel-box-parent`
- `camel-braintree`
- `camel-browse`
- `camel-caffeine`
- `camel-camunda`
- `camel-cassandraql`
- `camel-catalog`
- `camel-catalog-common`
- `camel-cbor`
- `camel-chatscript`
- `camel-chunk`
- `camel-cli-connector`
- `camel-cli-debug`
- `camel-clickhouse`
- `camel-clickup`
- `camel-cloudevents`
- `camel-cluster`
- `camel-cm-sms`
- `camel-coap`
- `camel-cometd`
- `camel-console`
- `camel-consul`
- `camel-controlbus`
- `camel-core`
- `camel-core-all`
- `camel-core-engine`
- `camel-core-languages`
- `camel-core-model`
- `camel-core-processor`
- `camel-core-reifier`
- `camel-core-xml`
- `camel-couchbase`
- `camel-couchdb`
- `camel-cron`
- `camel-crypto`
- `camel-crypto-pgp`
- `camel-csv`
- `camel-cxf-common`
- `camel-cxf-parent`
- `camel-cxf-rest`
- `camel-cxf-soap`
- `camel-cxf-spring-common`
- `camel-cxf-spring-rest`
- `camel-cxf-spring-soap`
- `camel-cxf-spring-transport`
- `camel-cxf-transport`
- `camel-cyberark-vault`
- `camel-dapr`
- `camel-dataformat`
- `camel-dataset`
- `camel-datasonnet`
- `camel-dataweave`
- `camel-debezium-common`
- `camel-debezium-common-parent`
- `camel-debezium-db2`
- `camel-debezium-maven-plugin`
- `camel-debezium-mongodb`
- `camel-debezium-mysql`
- `camel-debezium-oracle`
- `camel-debezium-parent`
- `camel-debezium-postgres`
- `camel-debezium-sqlserver`
- `camel-debug`
- `camel-dfdl`
- `camel-dhis2`
- `camel-dhis2-api`
- `camel-dhis2-parent`
- `camel-diagram`
- `camel-direct`
- `camel-disruptor`
- `camel-djl`
- `camel-dns`
- `camel-docker`
- `camel-docling`
- `camel-drill`
- `camel-dropbox`
- `camel-dsl-modeline`
- `camel-dsl-support`
- `camel-duckdb`
- `camel-dynamic-router`
- `camel-ehcache`
- `camel-elasticsearch`
- `camel-elasticsearch-rest-client`
- `camel-event`
- `camel-exec`
- `camel-fastjson`
- `camel-fhir`
- `camel-fhir-api`
- `camel-fhir-parent`
- `camel-file`
- `camel-file-watch`
- `camel-flatpack`
- `camel-flink`
- `camel-flowable`
- `camel-fop`
- `camel-fory`
- `camel-freemarker`
- `camel-ftp`
- `camel-ftp-common`
- `camel-geocoder`
- `camel-git`
- `camel-github2`
- `camel-google-bigquery`
- `camel-google-calendar`
- `camel-google-common`
- `camel-google-drive`
- `camel-google-firestore`
- `camel-google-functions`
- `camel-google-mail`
- `camel-google-parent`
- `camel-google-pubsub`
- `camel-google-secret-manager`
- `camel-google-sheets`
- `camel-google-speech-to-text`
- `camel-google-storage`
- `camel-google-text-to-speech`
- `camel-google-vertexai`
- `camel-google-vision`
- `camel-graphql`
- `camel-grok`
- `camel-groovy`
- `camel-grpc`
- `camel-gson`
- `camel-hashicorp-vault`
- `camel-hazelcast`
- `camel-health`
- `camel-hivemq`
- `camel-hl7`
- `camel-http`
- `camel-http-base`
- `camel-http-common`
- `camel-huawei-parent`
- `camel-huaweicloud-common`
- `camel-huaweicloud-dms`
- `camel-huaweicloud-frs`
- `camel-huaweicloud-functiongraph`
- `camel-huaweicloud-iam`
- `camel-huaweicloud-imagerecognition`
- `camel-huaweicloud-obs`
- `camel-huaweicloud-smn`
- `camel-huggingface`
- `camel-ibm-cos`
- `camel-ibm-parent`
- `camel-ibm-secrets-manager`
- `camel-ibm-watson-discovery`
- `camel-ibm-watson-language`
- `camel-ibm-watson-speech-to-text`
- `camel-ibm-watson-text-to-speech`
- `camel-ibm-watsonx-ai`
- `camel-ibm-watsonx-data`
- `camel-ical`
- `camel-iggy`
- `camel-ignite`
- `camel-infinispan`
- `camel-infinispan-common`
- `camel-infinispan-embedded`
- `camel-infinispan-parent`
- `camel-influxdb`
- `camel-influxdb2`
- `camel-iso8583`
- `camel-jackson`
- `camel-jackson-avro`
- `camel-jackson-protobuf`
- `camel-jackson3`
- `camel-jackson3-avro`
- `camel-jackson3-protobuf`
- `camel-jackson3xml`
- `camel-jacksonxml`
- `camel-jactl`
- `camel-jandex`
- `camel-jasypt`
- `camel-java-io`
- `camel-java-joor-dsl`
- `camel-javascript`
- `camel-jaxb`
- `camel-jbang-console`
- `camel-jbang-mcp`
- `camel-jbang-plugin-mcp`
- `camel-jbang-plugin-route-parser`
- `camel-jbang-plugin-tui`
- `camel-jbang-plugin-validate`
- `camel-jcache`
- `camel-jcr`
- `camel-jdbc`
- `camel-jetty`
- `camel-jetty-common`
- `camel-jfr`
- `camel-jgroups`
- `camel-jgroups-raft`
- `camel-jira`
- `camel-jms`
- `camel-jmx`
- `camel-jolt`
- `camel-jooq`
- `camel-joor`
- `camel-jpa`
- `camel-jq`
- `camel-jsch`
- `camel-jslt`
- `camel-json-validator`
- `camel-jsonapi`
- `camel-jsonata`
- `camel-jsonb`
- `camel-jsonpath`
- `camel-jsoup`
- `camel-jt400`
- `camel-jta`
- `camel-jte`
- `camel-kafka`
- `camel-kamelet`
- `camel-kamelet-main-support`
- `camel-keycloak`
- `camel-knative`
- `camel-knative-api`
- `camel-knative-http`
- `camel-knative-parent`
- `camel-kserve`
- `camel-kubernetes`
- `camel-kudu`
- `camel-langchain4j-agent`
- `camel-langchain4j-agent-api`
- `camel-langchain4j-chat`
- `camel-langchain4j-core`
- `camel-langchain4j-embeddings`
- `camel-langchain4j-embeddingstore`
- `camel-langchain4j-embeddingstore-api`
- `camel-langchain4j-tokenizer`
- `camel-langchain4j-web-search`
- `camel-language`
- `camel-launcher-container`
- `camel-ldap`
- `camel-ldif`
- `camel-log`
- `camel-lra`
- `camel-lucene`
- `camel-lumberjack`
- `camel-lzf`
- `camel-mail`
- `camel-mail-microsoft-oauth`
- `camel-main`
- `camel-management`
- `camel-mapstruct`
- `camel-master`
- `camel-maven-plugin`
- `camel-mcp-server`
- `camel-mcp-server-api`
- `camel-mdc`
- `camel-metrics`
- `camel-micrometer`
- `camel-micrometer-observability`
- `camel-micrometer-prometheus`
- `camel-microprofile-config`
- `camel-microprofile-fault-tolerance`
- `camel-microprofile-health`
- `camel-microprofile-parent`
- `camel-milo`
- `camel-milvus`
- `camel-mina`
- `camel-mina-sftp`
- `camel-minio`
- `camel-mllp`
- `camel-mock`
- `camel-mongodb`
- `camel-mongodb-gridfs`
- `camel-mustache`
- `camel-mvel`
- `camel-mybatis`
- `camel-nats`
- `camel-neo4j`
- `camel-netty`
- `camel-netty-http`
- `camel-oaipmh`
- `camel-oauth`
- `camel-observability-services`
- `camel-observation`
- `camel-ocsf`
- `camel-ognl`
- `camel-olingo2`
- `camel-olingo2-api`
- `camel-olingo2-parent`
- `camel-olingo4`
- `camel-olingo4-api`
- `camel-olingo4-parent`
- `camel-once`
- `camel-openai`
- `camel-openapi-java`
- `camel-openapi-rest-dsl-generator`
- `camel-openapi-validator`
- `camel-opensearch`
- `camel-openstack`
- `camel-opentelemetry`
- `camel-opentelemetry-metrics`
- `camel-opentelemetry2`
- `camel-optaplanner`
- `camel-paho`
- `camel-paho-mqtt5`
- `camel-parquet-avro`
- `camel-pdf`
- `camel-pg-replication-slot`
- `camel-pgevent`
- `camel-pgvector`
- `camel-pinecone`
- `camel-platform-http`
- `camel-platform-http-jolokia`
- `camel-platform-http-main`
- `camel-platform-http-vertx`
- `camel-plc4x`
- `camel-pqc`
- `camel-printer`
- `camel-protobuf`
- `camel-pubnub`
- `camel-pulsar`
- `camel-python`
- `camel-python3`
- `camel-qdrant`
- `camel-quartz`
- `camel-quickfix`
- `camel-quickjs`
- `camel-reactive-streams`
- `camel-reactor`
- `camel-redis`
- `camel-ref`
- `camel-resilience4j`
- `camel-resilience4j-micrometer`
- `camel-resourceresolver-github`
- `camel-rest`
- `camel-rest-openapi`
- `camel-rest-postman`
- `camel-restdsl-openapi-plugin`
- `camel-robotframework`
- `camel-rocketmq`
- `camel-rss`
- `camel-rxjava`
- `camel-saga`
- `camel-salesforce`
- `camel-salesforce-codegen`
- `camel-salesforce-maven-plugin`
- `camel-salesforce-parent`
- `camel-sap-netweaver`
- `camel-saxon`
- `camel-scheduler`
- `camel-schematron`
- `camel-seda`
- `camel-servicenow`
- `camel-servicenow-maven-plugin`
- `camel-servicenow-parent`
- `camel-servlet`
- `camel-shell`
- `camel-shiro`
- `camel-sjms`
- `camel-sjms2`
- `camel-slack`
- `camel-smb`
- `camel-smooks`
- `camel-smpp`
- `camel-snakeyaml`
- `camel-snmp`
- `camel-soap`
- `camel-solr`
- `camel-spiffe`
- `camel-splunk-hec`
- `camel-spring`
- `camel-spring-ai-chat`
- `camel-spring-ai-embeddings`
- `camel-spring-ai-image`
- `camel-spring-ai-parent`
- `camel-spring-ai-vector-store`
- `camel-spring-batch`
- `camel-spring-cloud-config`
- `camel-spring-jdbc`
- `camel-spring-ldap`
- `camel-spring-main`
- `camel-spring-parent`
- `camel-spring-rabbitmq`
- `camel-spring-redis`
- `camel-spring-security`
- `camel-spring-ws`
- `camel-spring-xml`
- `camel-sql`
- `camel-ssh`
- `camel-state-store`
- `camel-state-store-parent`
- `camel-stax`
- `camel-stitch`
- `camel-stream`
- `camel-streamcaching-test`
- `camel-stringtemplate`
- `camel-stripe`
- `camel-stub`
- `camel-support`
- `camel-swift`
- `camel-syslog`
- `camel-tahu`
- `camel-tarfile`
- `camel-telegram`
- `camel-telemetry`
- `camel-telemetry-dev`
- `camel-tensorflow-serving`
- `camel-test-infra-all`
- `camel-test-infra-artemis`
- `camel-test-infra-cli`
- `camel-test-infra-core`
- `camel-test-infra-smb`
- `camel-test-junit5`
- `camel-test-junit6`
- `camel-test-main-junit5`
- `camel-test-main-junit6`
- `camel-test-parent`
- `camel-test-spring-junit5`
- `camel-test-spring-junit6`
- `camel-thrift`
- `camel-thymeleaf`
- `camel-tika`
- `camel-timer`
- `camel-tooling-maven`
- `camel-toon`
- `camel-tracing`
- `camel-twilio`
- `camel-twitter`
- `camel-ubl`
- `camel-undertow`
- `camel-undertow-spring-security`
- `camel-univocity-parsers`
- `camel-validator`
- `camel-velocity`
- `camel-vertx`
- `camel-vertx-common`
- `camel-vertx-http`
- `camel-vertx-parent`
- `camel-vertx-websocket`
- `camel-wal`
- `camel-wasm`
- `camel-weather`
- `camel-weaviate`
- `camel-web3j`
- `camel-webhook`
- `camel-whatsapp`
- `camel-wordpress`
- `camel-workday`
- `camel-xchange`
- `camel-xj`
- `camel-xml-io`
- `camel-xml-io-dsl`
- `camel-xml-jaxb`
- `camel-xml-jaxb-dsl`
- `camel-xml-jaxb-dsl-test-definition`
- `camel-xml-jaxb-dsl-test-spring`
- `camel-xml-jaxp`
- `camel-xmlsecurity`
- `camel-xmpp`
- `camel-xpath`
- `camel-xslt`
- `camel-xslt-saxon`
- `camel-yaml-dsl-common`
- `camel-yaml-dsl-validator`
- `camel-yaml-dsl-validator-maven-plugin`
- `camel-yaml-io`
- `camel-zendesk`
- `camel-zip-deflater`
- `camel-zipfile`
- `camel-zookeeper`
- `camel-zookeeper-master`
- `components`
</details>
<details><summary>Modules with tests skipped (26)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
> :information_source: Shadow mode â Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
:warning: **Some tests are disabled on GitHub Actions** (`@DisabledIfSystemProperty(named = "ci.env.name")`) and require manual verification:
- `core/camel-core`: 2 test(s) disabled on GitHub Actions
<details><summary>Build reactor â dependencies compiled but only changed modules were tested (5 modules)</summary>
- Camel :: Catalog :: Camel Catalog
- Camel :: Core
- Camel :: Master
- Camel :: Simple JMS
- Camel :: Support
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/33963887304)
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3949740446
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,57 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext, BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ * <p/>
+ * This does not wait for an attempt that is already running: with {@code mayInterruptIfRunning} false, a supplier
+ * call that is in progress runs to completion after this method returns, and {@link #isAttempting()} keeps
+ * reporting it until it does.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
+ unschedule(mayInterruptIfRunning);
+ if (status == Status.Active) {
+ status = Status.Inactive;
+ }
+ completed.set(false);
+ deregister();
+ running.set(false);
+ }
+
Review Comment:
**isRunning() races with an in-progress attempt when mayInterruptIfRunning=false.**
When cancel(false) is called, the Javadoc correctly says isAttempting() keeps reporting true until the supplier finishes, but isRunning() is set to false here immediately, before that supplier call returns.
SimpleMessageListenerContainer (unchanged by this PR) gates on !recoverTask.isRunning() to decide whether to start a new recovery task (line 237). In practice, doStop() always passes cancel(true) and then nulls recoverTask, so the gap does not bite the SJMS case today. But as a public API contract, callers cannot rely on isRunning() to mean no attempt is currently executing - only isAttempting() can say that when cancel(false) was used.
At minimum, the Javadoc could say: After this method returns, isRunning() returns false even if an attempt is still in progress; use isAttempting() to check for that.
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,57 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext, BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ * <p/>
+ * This does not wait for an attempt that is already running: with {@code mayInterruptIfRunning} false, a supplier
+ * call that is in progress runs to completion after this method returns, and {@link #isAttempting()} keeps
+ * reporting it until it does.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
+ unschedule(mayInterruptIfRunning);
+ if (status == Status.Active) {
+ status = Status.Inactive;
+ }
+ completed.set(false);
+ deregister();
+ running.set(false);
Review Comment:
**completed.set(false) resets state on an already-completed task.**
If cancel() is called defensively on a task that finished successfully, completed is reset to false. In the current call sites (all on actively-retrying tasks) this does not matter, but since cancel() is now public API it is worth documenting or guarding. Consider completed.compareAndSet(true, false) or noting in the Javadoc that cancel() is only meaningful on a still-running task.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5635742576
<!-- camel-backport-bot -->
### đ Backport Bot
**Conflicts (đ¤ agent dispatched to resolve):**
- â ī¸ `camel-4.18.x` â Cherry-pick conflict in: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/master-component.adoc, components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java, components/camel-master/src/test/java/org/apache/camel/component/master/MasterConsumerLeadershipTest.java, core/camel-core/src/test/java/org/apache/camel/support/task/task/BackgroundTaskTest.java, core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java
âšī¸ If you push additional commits after `/port`, use `/port` again to update the port PRs.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5635812735
<!-- camel-backport-bot -->
### đ Backport Bot
**Conflicts (đ¤ agent dispatched to resolve):**
- â ī¸ `camel-4.18.x` â Command failed: git checkout -b backport/26112-to-camel-4.18.x
fatal: a branch named 'backport/26112-to-camel-4.18.x' already exists
âšī¸ If you push additional commits after `/port`, use `/port` again to update the port PRs.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3947935962
##########
components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java:
##########
@@ -109,14 +116,19 @@ protected void doStart() throws Exception {
protected void doStop() throws Exception {
super.doStop();
- // a start can still be pending, cancel it first so it cannot start the delegated consumer
- // after this consumer has been stopped
- leadershipTaken = false;
- cancelLeaderTask(true);
+ leadershipLock.lock();
+ try {
+ // a start can still be pending, cancel it first so it cannot start the delegated consumer
+ // after this consumer has been stopped
+ leadershipTaken = false;
+ cancelLeaderTask(true);
+ } finally {
+ leadershipLock.unlock();
+ }
- // note: removeEventListener below needs the cluster view lock while this thread holds the lock of
- // this service, which is the opposite order of an event dispatch. Nothing that runs under this lock
- // may wait for the view, and the listener bails out before locking once this consumer is stopping
+ // note: removeEventListener below needs the write lock of the cluster view, while an event dispatch
+ // takes the read lock of the view and then leadershipLock. This thread must not hold leadershipLock
+ // here, or the two orders deadlock
Review Comment:
stop() sets status = STOPPING before calling doStop() (`BaseService.java:173-176`), so the gate is closed regardless of super.doStop(). Adding that would record a cause that isn't real.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3950111421
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,57 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext, BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ * <p/>
+ * This does not wait for an attempt that is already running: with {@code mayInterruptIfRunning} false, a supplier
+ * call that is in progress runs to completion after this method returns, and {@link #isAttempting()} keeps
+ * reporting it until it does.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
+ unschedule(mayInterruptIfRunning);
+ if (status == Status.Active) {
+ status = Status.Inactive;
+ }
+ completed.set(false);
+ deregister();
+ running.set(false);
Review Comment:
Guarded: completed.set(false) moved under the existing status == Status.Active check, so a Completed, Failed or Exhausted task keeps its outcome, and run() cannot be made to return false for a task that succeeded. Note compareAndSet(true, false) would have performed the clobber rather than prevented it. Covered by testCancelKeepsTheOutcomeOfACompletedTask.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3947931113
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,53 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext, BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
Review Comment:
Added the javadoc note. isRunning() is the live schedule, not an executing supplier (`BlockingTask.java:62-67`) - isAttempting() is that one, and cancel() leaves it alone.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #26112:
URL: https://github.com/apache/camel/pull/26112
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5550500391
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5635944436
â
Backport to `camel-4.18.x` conflicts re-resolved and force-pushed: https://github.com/apache/camel/pull/26340
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3947938555
##########
components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java:
##########
@@ -315,8 +329,12 @@ private boolean isStartPending() {
}
private void cancelLeaderTask(boolean mayInterruptIfRunning) {
- if (leaderTaskFuture != null) {
- leaderTaskFuture.cancel(mayInterruptIfRunning);
+ if (leaderTask != null) {
+ // cancelled through the task and not through its future, so the task also leaves the
+ // TaskManagerRegistry. Only a run of the task removes it from there, and once the schedule
+ // is cancelled no run is coming
+ leaderTask.cancel(mayInterruptIfRunning);
+ leaderTask = null;
Review Comment:
Documented on the declarations, `MasterConsumer.java:66-70`. delegatedConsumer and view are volatile because they're read outside the lock; these two aren't.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5567855004
@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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "henrik242 (via GitHub)" <gi...@apache.org>.
henrik242 commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3950107225
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,57 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext, BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ * <p/>
+ * This does not wait for an attempt that is already running: with {@code mayInterruptIfRunning} false, a supplier
+ * call that is in progress runs to completion after this method returns, and {@link #isAttempting()} keeps
+ * reporting it until it does.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
+ unschedule(mayInterruptIfRunning);
+ if (status == Status.Active) {
+ status = Status.Inactive;
+ }
+ completed.set(false);
+ deregister();
+ running.set(false);
+ }
+
Review Comment:
Javadoc now says exactly that, BackgroundTask.java:189-192.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5572602715
<!-- camel-backport-bot -->
### đ Backport Bot
**Port PRs:**
- đ `camel-4.22.x` â #26172
This bugfix was merged into `main` and may need porting to:
- `camel-4.18.x`
Labels `port/<branch>` have been added. Remove a label to opt out of porting to that branch.
Port PRs will be created shortly.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3940597962
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,53 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext, BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
Review Comment:
đĄ **Observation (low):** `cancel()` sets `running.set(false)` at the end, but a concurrent `runTaskWrapper()` that is already past the latch check and executing the supplier will still be running. After `cancel()` returns, `isRunning()` returns `false` while an attempt may still be in progress (with `mayInterruptIfRunning=false`).
This is probably fine in practice â callers that need to wait for the in-flight attempt to finish would use `mayInterruptIfRunning=true` or await the future â but it means `isRunning()` can briefly lie after `cancel(false)`. Worth a comment on the Javadoc noting that `cancel(false)` does not wait for an in-flight supplier call to complete.
##########
components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java:
##########
@@ -315,8 +329,12 @@ private boolean isStartPending() {
}
private void cancelLeaderTask(boolean mayInterruptIfRunning) {
- if (leaderTaskFuture != null) {
- leaderTaskFuture.cancel(mayInterruptIfRunning);
+ if (leaderTask != null) {
+ // cancelled through the task and not through its future, so the task also leaves the
+ // TaskManagerRegistry. Only a run of the task removes it from there, and once the schedule
+ // is cancelled no run is coming
+ leaderTask.cancel(mayInterruptIfRunning);
+ leaderTask = null;
Review Comment:
đĄ **Observation (medium):** The `cancelLeaderTask` method nulls `leaderTask` and `leaderTaskFuture` but these fields are not volatile. They're always accessed under `leadershipLock`, so this is safe â but it's worth noting this invariant somewhere since the fields are declared next to the volatile `delegatedConsumer` and `view` at lines 64-65, and a future reader might wonder why some fields are volatile and others aren't.
The `leaderTask` field could arguably be volatile for consistency with the surrounding declarations, but since all accesses are lock-guarded it's unnecessary overhead.
##########
components/camel-master/src/test/java/org/apache/camel/component/master/MasterConsumerLeadershipTest.java:
##########
@@ -224,6 +230,66 @@ void testAllConfiguredStartAttemptsAreMade() {
assertEquals(0, probe.started.get());
}
+ @Test
+ @Timeout(60)
+ void testCancellingAPendingStartRemovesTheTaskFromTheRegistry() {
+ TestClusterView view = clusterService.getTestView();
+ TaskManagerRegistry registry = PluginHelper.getTaskManagerRegistry(context.getCamelContextExtension());
+ MasterComponent master = context.getComponent("master", MasterComponent.class);
+ // the task must still be retrying when the leadership is lost below, not exhausted by then
+ master.setBackOffMaxAttempts(1000);
+
+ probe.failStart.set(true);
+ view.setLeader(true);
+
+ // the task adds itself to the registry from its first run
+ await().atMost(20, TimeUnit.SECONDS).until(() -> probe.startAttempts.get() >= 1);
+ await().atMost(20, TimeUnit.SECONDS).until(() -> hasLeadershipTask(registry));
+
+ view.setLeader(false);
+
+ // only a run of the task removes it from the registry, and after the cancel no run is coming
+ await().atMost(20, TimeUnit.SECONDS).untilAsserted(() -> assertFalse(hasLeadershipTask(registry),
+ "The cancelled start task must not stay in the task registry"));
+ }
+
+ @Test
+ @Timeout(60)
+ void testEventDispatchIsNotBlockedByALifecycleOperation() throws Exception {
+ TestClusterView view = clusterService.getTestView();
+
+ view.setLeader(true);
+ await().atMost(10, TimeUnit.SECONDS).untilAsserted(() -> assertEquals(1, probe.started.get()));
+
+ CountDownLatch suspendEntered = new CountDownLatch(1);
+ CountDownLatch suspendGate = new CountDownLatch(1);
+ probe.suspendEntered.set(suspendEntered);
+ probe.suspendGate.set(suspendGate);
+
+ // suspending holds the service lock of the master consumer for as long as the delegate takes
+ MasterConsumer consumer = (MasterConsumer) context.getRoute("master-route").getConsumer();
+ Thread suspender = new Thread(consumer::suspend, "suspend");
+ suspender.start();
+ assertTrue(suspendEntered.await(20, TimeUnit.SECONDS), "The suspend of the delegate should have started");
+
+ // the cluster view dispatches its events while holding its own lock, and needs that same lock again
+ // to remove the listener when the consumer stops. An event that waits here for the service lock of
+ // the consumer is what closes that into a deadlock, so the leadership must not be guarded by it
+ Thread dispatcher = new Thread(() -> view.setLeader(true), "leadership-taken");
+ dispatcher.start();
+ try {
+ dispatcher.join(TimeUnit.SECONDS.toMillis(20));
Review Comment:
đĄ **Nice test.** `testEventDispatchIsNotBlockedByALifecycleOperation` is a well-designed deadlock detection test â holding the service lock via a blocking suspend, then verifying the dispatch thread completes without waiting. The `join(20s)` timeout with the alive check is the right pattern for this.
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,53 @@ public Future<?> schedule(CamelContext camelContext, BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
Review Comment:
đĄ **Observation (low):** `scheduledContext` is set after `scheduledFuture` (line 174 vs 173), but `cancel()` reads `scheduledContext` in `deregister()` and `scheduledFuture` in `unschedule()` independently. There's a theoretical window where `cancel()` is called between the two `set()` calls â it would `unschedule` the future but `deregister()` would see a null context and skip the registry removal.
The race guard in `runTaskWrapper` (lines 108-113) catches this: a run that started before the cancel saw the latch will deregister on its next check. And the existing `if (latch.getCount() == 0) { unschedule(false); }` at line 176 catches the reverse. So the window is covered by defense-in-depth, but it might be slightly cleaner to set both atomically (or at least set `scheduledContext` first, before `scheduledFuture`, since `cancel()` checks the future first).
Looking again â `scheduledContext` IS set before `scheduledFuture` at lines 173-174. So if `cancel()` fires between them, it sees the context (deregisters) but doesn't see the future yet (skips unschedule). Then line 174 publishes the future, and the final `latch.getCount() == 0` check at line 176 catches it. đ Order is correct.
##########
components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java:
##########
@@ -109,14 +116,19 @@ protected void doStart() throws Exception {
protected void doStop() throws Exception {
super.doStop();
- // a start can still be pending, cancel it first so it cannot start the delegated consumer
- // after this consumer has been stopped
- leadershipTaken = false;
- cancelLeaderTask(true);
+ leadershipLock.lock();
+ try {
+ // a start can still be pending, cancel it first so it cannot start the delegated consumer
+ // after this consumer has been stopped
+ leadershipTaken = false;
+ cancelLeaderTask(true);
+ } finally {
+ leadershipLock.unlock();
+ }
- // note: removeEventListener below needs the cluster view lock while this thread holds the lock of
- // this service, which is the opposite order of an event dispatch. Nothing that runs under this lock
- // may wait for the view, and the listener bails out before locking once this consumer is stopping
+ // note: removeEventListener below needs the write lock of the cluster view, while an event dispatch
+ // takes the read lock of the view and then leadershipLock. This thread must not hold leadershipLock
+ // here, or the two orders deadlock
Review Comment:
đĄ **Observation (low):** `doStop()` acquires `leadershipLock`, clears state, releases the lock, then calls `view.removeEventListener()`. Between the lock release and the `removeEventListener`, a leadership event could arrive, pass the `isRunAllowed()` fast check (which returns `false` since `super.doStop()` ran first), and bail out â correct. But the comment on line 128-131 explaining this ordering is valuable. It might be worth adding that `super.doStop()` has already run at this point, so `isRunAllowed()` is the first gate the listener hits.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26112:
URL: https://github.com/apache/camel/pull/26112#issuecomment-5635867856
â
Backport to `camel-4.18.x` created: https://github.com/apache/camel/pull/26340
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org