You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "luigidemasi (via GitHub)" <gi...@apache.org> on 2023/09/05 16:43:53 UTC
[GitHub] [camel] luigidemasi opened a new pull request, #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
luigidemasi opened a new pull request, #11307:
URL: https://github.com/apache/camel/pull/11307
(no comment)
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1316205940
##########
core/camel-util/src/main/java/org/apache/camel/util/ReflectionHelper.java:
##########
@@ -177,7 +177,7 @@ public static Method findMethod(Class<?> clazz, String name, Class<?>... paramTy
return null;
}
- public static void setField(Field f, Object instance, Object value) {
+ public synchronized static void setField(Field f, Object instance, Object value) {
Review Comment:
It is not exactly what I had in mind, I was more thinking of removing the code that restores the old value of the flag this way we don't need any synchronization mechanism but we can wait for comments from others
--
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
[GitHub] [camel] essobedo merged pull request #11307: CAMEL-19833: camel-bindy: race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo merged PR #11307:
URL: https://github.com/apache/camel/pull/11307
--
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
[GitHub] [camel] github-actions[bot] commented on pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11307:
URL: https://github.com/apache/camel/pull/11307#issuecomment-1706961160
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:camel: Maintainers, please note that first-time contributors *require manual approval* for the GitHub Actions to run.
:warning: Please note that the changes on this PR may be **tested automatically** if they change components.
:robot: Use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot.
If necessary Apache Camel Committers may access logs and test results in the job summaries!
--
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
[GitHub] [camel] luigidemasi commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1316190781
##########
components/camel-bindy/src/main/java/org/apache/camel/dataformat/bindy/BindyAbstractFactory.java:
##########
@@ -133,7 +133,7 @@ public abstract void bind(CamelContext camelContext, List<String> data, Map<Stri
/**
* Link objects together
*/
- public void link(Map<String, Object> model) {
+ public synchronized void link(Map<String, Object> model) {
Review Comment:
It was my first idea to fix it at ReflectionHelper level but I wanted my changes to impact as less as possible on other components, but yeah the scenario you described, even uncommon, can be a problem. I will change the commit.
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1316980632
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.stream.Stream;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.io.TempDir;
+
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ @TempDir
+ Path tempDir;
+
+ String SOURCE_DIRECTORY_PATH = "org/apache/camel/dataformat/bindy/fixed/link";
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+ copyFiles();
+ MockEndpoint end = getMockEndpoint("mock:end");
+ MockEndpoint fail = getMockEndpoint("mock:fail");
+ end.expectedMessageCount(10);
+ fail.expectedMessageCount(0);
+ end.assertIsSatisfied(5000);
+ fail.assertIsSatisfied(5000);
+ assertEquals(0, end.getFailures().size());
+ end.reset();
+ fail.reset();
+ }
+
+ private void copyFiles() throws IOException, URISyntaxException {
+ try (Stream<Path> stream = Files.list(Path.of(ClassLoader.getSystemResource(SOURCE_DIRECTORY_PATH).toURI()))) {
+ List<Path> paths = stream.filter(path -> path.toString().endsWith(".csv")).toList();
+ for (Path source : paths) {
+ Path destination = Paths.get(tempDir.toString() + File.separator + source.getFileName());
+ Files.copy(source, destination, REPLACE_EXISTING);
+ }
+ }
+ }
+
+ @Override
+ public boolean isUseRouteBuilder() {
+ return true;
+ }
Review Comment:
Do we really need it? I think it is the default value
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.stream.Stream;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.io.TempDir;
+
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ @TempDir
+ Path tempDir;
+
+ String SOURCE_DIRECTORY_PATH = "org/apache/camel/dataformat/bindy/fixed/link";
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+ copyFiles();
+ MockEndpoint end = getMockEndpoint("mock:end");
+ MockEndpoint fail = getMockEndpoint("mock:fail");
+ end.expectedMessageCount(10);
+ fail.expectedMessageCount(0);
+ end.assertIsSatisfied(5000);
+ fail.assertIsSatisfied(5000);
+ assertEquals(0, end.getFailures().size());
+ end.reset();
+ fail.reset();
+ }
+
+ private void copyFiles() throws IOException, URISyntaxException {
+ try (Stream<Path> stream = Files.list(Path.of(ClassLoader.getSystemResource(SOURCE_DIRECTORY_PATH).toURI()))) {
+ List<Path> paths = stream.filter(path -> path.toString().endsWith(".csv")).toList();
+ for (Path source : paths) {
+ Path destination = Paths.get(tempDir.toString() + File.separator + source.getFileName());
+ Files.copy(source, destination, REPLACE_EXISTING);
+ }
+ }
Review Comment:
Do we really need 10 big CSV files to reproduce? Maybe we could have only one file, split it into lines then parallelize the processing with `.split(body()).delimiter("\n").parallelProcessing()`, WDYT?
##########
core/camel-util/src/main/java/org/apache/camel/util/ReflectionHelper.java:
##########
@@ -179,20 +179,10 @@ public static Method findMethod(Class<?> clazz, String name, Class<?>... paramTy
public static void setField(Field f, Object instance, Object value) {
try {
- boolean oldAccessible = false;
- try {
- oldAccessible = f.canAccess(instance);
- } catch (Exception e) {
- // ignore
- }
- boolean shouldSetAccessible = !Modifier.isPublic(f.getModifiers()) && !oldAccessible;
- if (shouldSetAccessible) {
+ if (!Modifier.isPublic(f.getModifiers()) && !f.canAccess(instance)) {
Review Comment:
Yes, that sounds better. Maybe, we could do the same with `getField`, don't you agree?
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/model/fixed/link/MyModel.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.camel.dataformat.bindy.model.fixed.link;
+
+import org.apache.camel.dataformat.bindy.annotation.CsvRecord;
+import org.apache.camel.dataformat.bindy.annotation.DataField;
+import org.apache.camel.dataformat.bindy.annotation.Link;
+
+@CsvRecord(separator = ",", skipField = true)
+public class MyModel {
Review Comment:
To increase the chance of collision, maybe we should only have one single private field instead of many different fields, don't you agree?
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1317229845
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ String SOURCE_CSV_FILE_PATH = "org/apache/camel/dataformat/bindy/fixed/link/bindyRaceConditionLinkTest.csv";
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+
+ Path filePath = Path.of(ClassLoader.getSystemResource(SOURCE_CSV_FILE_PATH).toURI());
+ String csv = Files.readString(filePath);
+ template.requestBody("direct:bindy-link-test", csv);
+
+ MockEndpoint end = getMockEndpoint("mock:end");
+ MockEndpoint fail = getMockEndpoint("mock:fail");
+ fail.expectedMessageCount(0);
+ fail.assertIsSatisfied(5000);
+ assertEquals(0, end.getFailures().size());
+ end.reset();
+ fail.reset();
+ }
+
+ @Override
+ public RouteBuilder createRouteBuilder() {
+ return new RouteBuilder() {
+ @Override
+ public void configure() {
+ BindyCsvDataFormat bindy = new BindyCsvDataFormat(MyModel.class);
+
+ ExecutorService executorService = new ThreadPoolExecutor(
+ 10,
+ 10,
+ 15,
+ TimeUnit.SECONDS,
+ new ArrayBlockingQueue<>(11000));
+
+ onException(UnsupportedOperationException.class).to("mock:fail");
+
+ from("direct:bindy-link-test")
+ .routeId("bindy-link-test")
+ .convertBodyTo(String.class)
+ .split(body().tokenize("\\n"))
+ .parallelProcessing(true)
+ .executorService(executorService)
+ .log("will unmarshal")
Review Comment:
Let's remove the log messages that slow down the test and pollute the log file
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ String SOURCE_CSV_FILE_PATH = "org/apache/camel/dataformat/bindy/fixed/link/bindyRaceConditionLinkTest.csv";
Review Comment:
Should be `static final`
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/model/fixed/link/MyModel.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.camel.dataformat.bindy.model.fixed.link;
+
+import org.apache.camel.dataformat.bindy.annotation.CsvRecord;
+import org.apache.camel.dataformat.bindy.annotation.DataField;
+import org.apache.camel.dataformat.bindy.annotation.Link;
+
+@CsvRecord(separator = ",", skipField = true)
+public class MyModel {
Review Comment:
Could you please tell me why we need many fields to reproduce? I insist on that because adding big files to a github project tends to slow down different git commands so it should be done only when really needed.
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ String SOURCE_CSV_FILE_PATH = "org/apache/camel/dataformat/bindy/fixed/link/bindyRaceConditionLinkTest.csv";
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+
+ Path filePath = Path.of(ClassLoader.getSystemResource(SOURCE_CSV_FILE_PATH).toURI());
+ String csv = Files.readString(filePath);
+ template.requestBody("direct:bindy-link-test", csv);
+
+ MockEndpoint end = getMockEndpoint("mock:end");
+ MockEndpoint fail = getMockEndpoint("mock:fail");
+ fail.expectedMessageCount(0);
+ fail.assertIsSatisfied(5000);
+ assertEquals(0, end.getFailures().size());
+ end.reset();
+ fail.reset();
+ }
+
+ @Override
+ public RouteBuilder createRouteBuilder() {
+ return new RouteBuilder() {
+ @Override
+ public void configure() {
+ BindyCsvDataFormat bindy = new BindyCsvDataFormat(MyModel.class);
+
+ ExecutorService executorService = new ThreadPoolExecutor(
+ 10,
+ 10,
+ 15,
+ TimeUnit.SECONDS,
+ new ArrayBlockingQueue<>(11000));
+
+ onException(UnsupportedOperationException.class).to("mock:fail");
+
+ from("direct:bindy-link-test")
+ .routeId("bindy-link-test")
+ .convertBodyTo(String.class)
+ .split(body().tokenize("\\n"))
Review Comment:
Why do we need 2 backslashes?
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ String SOURCE_CSV_FILE_PATH = "org/apache/camel/dataformat/bindy/fixed/link/bindyRaceConditionLinkTest.csv";
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+
+ Path filePath = Path.of(ClassLoader.getSystemResource(SOURCE_CSV_FILE_PATH).toURI());
+ String csv = Files.readString(filePath);
+ template.requestBody("direct:bindy-link-test", csv);
+
+ MockEndpoint end = getMockEndpoint("mock:end");
+ MockEndpoint fail = getMockEndpoint("mock:fail");
+ fail.expectedMessageCount(0);
Review Comment:
This makes me wonder if the test is properly written because if you expect no message, the condition is already met even before starting the Camel route. You should rather expect that `mock:end` receives all the messages. I think that MockEndpoint should not be needed
--
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
[GitHub] [camel] essobedo commented on pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on PR #11307:
URL: https://github.com/apache/camel/pull/11307#issuecomment-1707889861
@luigidemasi Did you see my last comment?
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1318438857
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+
+import org.apache.camel.Endpoint;
+import org.apache.camel.EndpointInject;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ private static final String SOURCE_CSV_FILE_PATH
+ = "org/apache/camel/dataformat/bindy/fixed/link/bindyRaceConditionLinkTest.csv";
+
+ private static final int EXPECTED_SUCCESSFUL_MESSAGE_COUNT = 1500;
+ private static final int EXPECTED_FAILED_MESSAGE_COUNT = 0;
+ private static final long WAIT_TIMEOUT = 5;
+ private static final int THREAD_POOL_CORE_SIZE = 10;
+ private static final int THREAD_POOL_MAX_SIZE = 10;
+ private static final int THREAD_POOL_KEEP_ALIVE_TIME = 15;
+ private static final int THREAD_POOL_QUEUE_SIZE = 2000;
+
+ @EndpointInject("mock:fail")
+ private MockEndpoint fail;
+
+ @EndpointInject("mock:end")
+ private MockEndpoint end;
+
+ @EndpointInject("direct:bindy-link-test")
+ private Endpoint begin;
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+ Path filePath = Path.of(ClassLoader.getSystemResource(SOURCE_CSV_FILE_PATH).toURI());
+ String csv = Files.readString(filePath);
+ template.sendBody(begin, csv);
+ fail.expectedMessageCount(EXPECTED_FAILED_MESSAGE_COUNT);
+ end.expectedMessageCount(EXPECTED_SUCCESSFUL_MESSAGE_COUNT);
+ MockEndpoint.assertIsSatisfied(context, WAIT_TIMEOUT, SECONDS);
+ end.reset();
+ fail.reset();
+ }
+
+ @Override
+ public RouteBuilder createRouteBuilder() {
+ return new RouteBuilder() {
+ @Override
+ public void configure() {
+ BindyCsvDataFormat bindy = new BindyCsvDataFormat(MyModel.class);
+
+ ExecutorService executorService = new ThreadPoolExecutor(
+ THREAD_POOL_CORE_SIZE,
+ THREAD_POOL_MAX_SIZE,
+ THREAD_POOL_KEEP_ALIVE_TIME,
+ SECONDS,
+ new ArrayBlockingQueue<>(THREAD_POOL_QUEUE_SIZE));
Review Comment:
Did you check if your test fails without your custom `ExecutorService` and the bug fix? If you really need it make sure that it is shutdown properly at the end of the test to free up the memory. I can't remember if it is already done out of the box by Camel.
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1318438857
##########
components/camel-bindy/src/test/java/org/apache/camel/dataformat/bindy/fixed/link/BindyRaceConditionLinkTest.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.fixed.link;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+
+import org.apache.camel.Endpoint;
+import org.apache.camel.EndpointInject;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.dataformat.bindy.csv.BindyCsvDataFormat;
+import org.apache.camel.dataformat.bindy.model.fixed.link.MyModel;
+import org.apache.camel.test.junit5.CamelTestSupport;
+import org.junit.jupiter.api.RepeatedTest;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+/** Test for CAMEL-19833 **/
+public class BindyRaceConditionLinkTest extends CamelTestSupport {
+
+ private static final String SOURCE_CSV_FILE_PATH
+ = "org/apache/camel/dataformat/bindy/fixed/link/bindyRaceConditionLinkTest.csv";
+
+ private static final int EXPECTED_SUCCESSFUL_MESSAGE_COUNT = 1500;
+ private static final int EXPECTED_FAILED_MESSAGE_COUNT = 0;
+ private static final long WAIT_TIMEOUT = 5;
+ private static final int THREAD_POOL_CORE_SIZE = 10;
+ private static final int THREAD_POOL_MAX_SIZE = 10;
+ private static final int THREAD_POOL_KEEP_ALIVE_TIME = 15;
+ private static final int THREAD_POOL_QUEUE_SIZE = 2000;
+
+ @EndpointInject("mock:fail")
+ private MockEndpoint fail;
+
+ @EndpointInject("mock:end")
+ private MockEndpoint end;
+
+ @EndpointInject("direct:bindy-link-test")
+ private Endpoint begin;
+
+ @RepeatedTest(3)
+ public void raceConditionTest() throws Exception {
+ Path filePath = Path.of(ClassLoader.getSystemResource(SOURCE_CSV_FILE_PATH).toURI());
+ String csv = Files.readString(filePath);
+ template.sendBody(begin, csv);
+ fail.expectedMessageCount(EXPECTED_FAILED_MESSAGE_COUNT);
+ end.expectedMessageCount(EXPECTED_SUCCESSFUL_MESSAGE_COUNT);
+ MockEndpoint.assertIsSatisfied(context, WAIT_TIMEOUT, SECONDS);
+ end.reset();
+ fail.reset();
+ }
+
+ @Override
+ public RouteBuilder createRouteBuilder() {
+ return new RouteBuilder() {
+ @Override
+ public void configure() {
+ BindyCsvDataFormat bindy = new BindyCsvDataFormat(MyModel.class);
+
+ ExecutorService executorService = new ThreadPoolExecutor(
+ THREAD_POOL_CORE_SIZE,
+ THREAD_POOL_MAX_SIZE,
+ THREAD_POOL_KEEP_ALIVE_TIME,
+ SECONDS,
+ new ArrayBlockingQueue<>(THREAD_POOL_QUEUE_SIZE));
Review Comment:
Did you check if your test fails without your custom `ExecutorService` and the bug fix? If you really need it make sure that it is shutdown properly at the end of the test to free up the memory. I can't remember if is done out of the box by Camel.
--
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
[GitHub] [camel] essobedo commented on a diff in pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #11307:
URL: https://github.com/apache/camel/pull/11307#discussion_r1316162461
##########
components/camel-bindy/src/main/java/org/apache/camel/dataformat/bindy/BindyAbstractFactory.java:
##########
@@ -133,7 +133,7 @@ public abstract void bind(CamelContext camelContext, List<String> data, Map<Stri
/**
* Link objects together
*/
- public void link(Map<String, Object> model) {
+ public synchronized void link(Map<String, Object> model) {
Review Comment:
If we use a second `BindyCsvDataFormat` in our route on the same model but with different parameters will it be good enough to fix the problem? Generally speaking, I'm wondering if it should not be fixed at `ReflectionHelper` level by simply not restoring the flag. WDYT?
--
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
[GitHub] [camel] luigidemasi commented on pull request #11307: CAMEL-19833: camel-bindy - race condition in BindyAbstractFactory.link()
Posted by "luigidemasi (via GitHub)" <gi...@apache.org>.
luigidemasi commented on PR #11307:
URL: https://github.com/apache/camel/pull/11307#issuecomment-1707962662
> @luigidemasi Did you see [my last comment](https://github.com/apache/camel/pull/11307#discussion_r1316205940)?
yes, I modified the commit accordingly.
--
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