You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by on...@apache.org on 2017/07/28 08:27:26 UTC
camel git commit: CAMEL-11609:thread-safety if headers get modified
on the fly
Repository: camel
Updated Branches:
refs/heads/master 6d0fdee12 -> 27d6c83ec
CAMEL-11609:thread-safety if headers get modified on the fly
Project: http://git-wip-us.apache.org/repos/asf/camel/repo
Commit: http://git-wip-us.apache.org/repos/asf/camel/commit/27d6c83e
Tree: http://git-wip-us.apache.org/repos/asf/camel/tree/27d6c83e
Diff: http://git-wip-us.apache.org/repos/asf/camel/diff/27d6c83e
Branch: refs/heads/master
Commit: 27d6c83ecd392168e59971f5de8e4ad258a1f461
Parents: 6d0fdee
Author: onders86 <on...@gmail.com>
Authored: Thu Jul 27 23:54:18 2017 +0300
Committer: onders86 <on...@gmail.com>
Committed: Fri Jul 28 11:27:06 2017 +0300
----------------------------------------------------------------------
.../org/apache/camel/dataformat/univocity/Marshaller.java | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/camel/blob/27d6c83e/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
----------------------------------------------------------------------
diff --git a/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java b/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
index 5c113cb..e170c95 100644
--- a/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
+++ b/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
@@ -46,7 +46,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
*/
Marshaller(String[] headers, boolean adaptHeaders) {
if (headers != null) {
- this.headers.addAll(Arrays.asList(headers));
+ synchronized (this.headers) {
+ this.headers.addAll(Arrays.asList(headers));
+ }
}
this.adaptHeaders = adaptHeaders;
}
@@ -86,7 +88,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
Map<?, ?> map = convertToMandatoryType(exchange, Map.class, row);
if (adaptHeaders) {
for (Object key : map.keySet()) {
- headers.add(convertToMandatoryType(exchange, String.class, key));
+ synchronized (headers) {
+ headers.add(convertToMandatoryType(exchange, String.class, key));
+ }
}
}
Re: camel git commit: CAMEL-11609:thread-safety if headers get
modified on the fly
Posted by Onder SEZGIN <on...@gmail.com>.
Hi,
I say 0 for my commit :)
Because it is currently not thread safe and sync will help to ensure it
thread-safe modification of the list..
As discussed in the issue and PR as well, with this commit we can ensure
thread-safety for the modification of the list
We may discuss if this is a right approach to evaluate the data format
design, but this is not the point i suppose.
Please see my comment. (link
<https://issues.apache.org/jira/browse/CAMEL-11609?focusedCommentId=16103900&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16103900>
)
and my comment in the PR as well. (link
<https://github.com/apache/camel/pull/1855#discussion_r130071814>)
On Fri, Jul 28, 2017 at 1:56 PM, Claus Ibsen <cl...@gmail.com> wrote:
> Hi
>
> Yeah the data format implementation of marshal / unmarshal should be
> written in a thread-safe manner. So I think the current code in
> univocity may be wrong, if it has some kind of shared state for some
> headers.
>
>
>
> On Fri, Jul 28, 2017 at 10:51 AM, Zoran Regvart <zo...@regvart.com> wrote:
> > Hi Cameleers,
> >
> > -1
> >
> > not sure that was the correct solution, perhaps `headers` field of
> > Marshaller needs to be treated as immutable. Hard to tell on such a
> > rarely used component what was the initial intent of `headers` field,
> > but I guess it's what all marshallers need to add, so mutating it on a
> > per write seems wrong.
> >
> > Any thoughts?
> >
> > zoran
> >
> > On Fri, Jul 28, 2017 at 10:27 AM, <on...@apache.org> wrote:
> >> Repository: camel
> >> Updated Branches:
> >> refs/heads/master 6d0fdee12 -> 27d6c83ec
> >>
> >>
> >> CAMEL-11609:thread-safety if headers get modified on the fly
> >>
> >>
> >> Project: http://git-wip-us.apache.org/repos/asf/camel/repo
> >> Commit: http://git-wip-us.apache.org/repos/asf/camel/commit/27d6c83e
> >> Tree: http://git-wip-us.apache.org/repos/asf/camel/tree/27d6c83e
> >> Diff: http://git-wip-us.apache.org/repos/asf/camel/diff/27d6c83e
> >>
> >> Branch: refs/heads/master
> >> Commit: 27d6c83ecd392168e59971f5de8e4ad258a1f461
> >> Parents: 6d0fdee
> >> Author: onders86 <on...@gmail.com>
> >> Authored: Thu Jul 27 23:54:18 2017 +0300
> >> Committer: onders86 <on...@gmail.com>
> >> Committed: Fri Jul 28 11:27:06 2017 +0300
> >>
> >> ----------------------------------------------------------------------
> >> .../org/apache/camel/dataformat/univocity/Marshaller.java | 8
> ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >> ----------------------------------------------------------------------
> >>
> >>
> >> http://git-wip-us.apache.org/repos/asf/camel/blob/27d6c83e/
> components/camel-univocity-parsers/src/main/java/org/
> apache/camel/dataformat/univocity/Marshaller.java
> >> ----------------------------------------------------------------------
> >> diff --git a/components/camel-univocity-parsers/src/main/java/org/
> apache/camel/dataformat/univocity/Marshaller.java
> b/components/camel-univocity-parsers/src/main/java/org/
> apache/camel/dataformat/univocity/Marshaller.java
> >> index 5c113cb..e170c95 100644
> >> --- a/components/camel-univocity-parsers/src/main/java/org/
> apache/camel/dataformat/univocity/Marshaller.java
> >> +++ b/components/camel-univocity-parsers/src/main/java/org/
> apache/camel/dataformat/univocity/Marshaller.java
> >> @@ -46,7 +46,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
> >> */
> >> Marshaller(String[] headers, boolean adaptHeaders) {
> >> if (headers != null) {
> >> - this.headers.addAll(Arrays.asList(headers));
> >> + synchronized (this.headers) {
> >> + this.headers.addAll(Arrays.asList(headers));
> >> + }
> >> }
> >> this.adaptHeaders = adaptHeaders;
> >> }
> >> @@ -86,7 +88,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
> >> Map<?, ?> map = convertToMandatoryType(exchange, Map.class,
> row);
> >> if (adaptHeaders) {
> >> for (Object key : map.keySet()) {
> >> - headers.add(convertToMandatoryType(exchange,
> String.class, key));
> >> + synchronized (headers) {
> >> + headers.add(convertToMandatoryType(exchange,
> String.class, key));
> >> + }
> >> }
> >> }
> >>
> >>
> >
> >
> >
> > --
> > Zoran Regvart
>
>
>
> --
> Claus Ibsen
> -----------------
> http://davsclaus.com @davsclaus
> Camel in Action 2: https://www.manning.com/ibsen2
>
Re: camel git commit: CAMEL-11609:thread-safety if headers get
modified on the fly
Posted by Claus Ibsen <cl...@gmail.com>.
Hi
Yeah the data format implementation of marshal / unmarshal should be
written in a thread-safe manner. So I think the current code in
univocity may be wrong, if it has some kind of shared state for some
headers.
On Fri, Jul 28, 2017 at 10:51 AM, Zoran Regvart <zo...@regvart.com> wrote:
> Hi Cameleers,
>
> -1
>
> not sure that was the correct solution, perhaps `headers` field of
> Marshaller needs to be treated as immutable. Hard to tell on such a
> rarely used component what was the initial intent of `headers` field,
> but I guess it's what all marshallers need to add, so mutating it on a
> per write seems wrong.
>
> Any thoughts?
>
> zoran
>
> On Fri, Jul 28, 2017 at 10:27 AM, <on...@apache.org> wrote:
>> Repository: camel
>> Updated Branches:
>> refs/heads/master 6d0fdee12 -> 27d6c83ec
>>
>>
>> CAMEL-11609:thread-safety if headers get modified on the fly
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/camel/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/camel/commit/27d6c83e
>> Tree: http://git-wip-us.apache.org/repos/asf/camel/tree/27d6c83e
>> Diff: http://git-wip-us.apache.org/repos/asf/camel/diff/27d6c83e
>>
>> Branch: refs/heads/master
>> Commit: 27d6c83ecd392168e59971f5de8e4ad258a1f461
>> Parents: 6d0fdee
>> Author: onders86 <on...@gmail.com>
>> Authored: Thu Jul 27 23:54:18 2017 +0300
>> Committer: onders86 <on...@gmail.com>
>> Committed: Fri Jul 28 11:27:06 2017 +0300
>>
>> ----------------------------------------------------------------------
>> .../org/apache/camel/dataformat/univocity/Marshaller.java | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>> http://git-wip-us.apache.org/repos/asf/camel/blob/27d6c83e/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
>> ----------------------------------------------------------------------
>> diff --git a/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java b/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
>> index 5c113cb..e170c95 100644
>> --- a/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
>> +++ b/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
>> @@ -46,7 +46,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
>> */
>> Marshaller(String[] headers, boolean adaptHeaders) {
>> if (headers != null) {
>> - this.headers.addAll(Arrays.asList(headers));
>> + synchronized (this.headers) {
>> + this.headers.addAll(Arrays.asList(headers));
>> + }
>> }
>> this.adaptHeaders = adaptHeaders;
>> }
>> @@ -86,7 +88,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
>> Map<?, ?> map = convertToMandatoryType(exchange, Map.class, row);
>> if (adaptHeaders) {
>> for (Object key : map.keySet()) {
>> - headers.add(convertToMandatoryType(exchange, String.class, key));
>> + synchronized (headers) {
>> + headers.add(convertToMandatoryType(exchange, String.class, key));
>> + }
>> }
>> }
>>
>>
>
>
>
> --
> Zoran Regvart
--
Claus Ibsen
-----------------
http://davsclaus.com @davsclaus
Camel in Action 2: https://www.manning.com/ibsen2
Re: camel git commit: CAMEL-11609:thread-safety if headers get
modified on the fly
Posted by Zoran Regvart <zo...@regvart.com>.
Hi Cameleers,
-1
not sure that was the correct solution, perhaps `headers` field of
Marshaller needs to be treated as immutable. Hard to tell on such a
rarely used component what was the initial intent of `headers` field,
but I guess it's what all marshallers need to add, so mutating it on a
per write seems wrong.
Any thoughts?
zoran
On Fri, Jul 28, 2017 at 10:27 AM, <on...@apache.org> wrote:
> Repository: camel
> Updated Branches:
> refs/heads/master 6d0fdee12 -> 27d6c83ec
>
>
> CAMEL-11609:thread-safety if headers get modified on the fly
>
>
> Project: http://git-wip-us.apache.org/repos/asf/camel/repo
> Commit: http://git-wip-us.apache.org/repos/asf/camel/commit/27d6c83e
> Tree: http://git-wip-us.apache.org/repos/asf/camel/tree/27d6c83e
> Diff: http://git-wip-us.apache.org/repos/asf/camel/diff/27d6c83e
>
> Branch: refs/heads/master
> Commit: 27d6c83ecd392168e59971f5de8e4ad258a1f461
> Parents: 6d0fdee
> Author: onders86 <on...@gmail.com>
> Authored: Thu Jul 27 23:54:18 2017 +0300
> Committer: onders86 <on...@gmail.com>
> Committed: Fri Jul 28 11:27:06 2017 +0300
>
> ----------------------------------------------------------------------
> .../org/apache/camel/dataformat/univocity/Marshaller.java | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/camel/blob/27d6c83e/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
> ----------------------------------------------------------------------
> diff --git a/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java b/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
> index 5c113cb..e170c95 100644
> --- a/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
> +++ b/components/camel-univocity-parsers/src/main/java/org/apache/camel/dataformat/univocity/Marshaller.java
> @@ -46,7 +46,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
> */
> Marshaller(String[] headers, boolean adaptHeaders) {
> if (headers != null) {
> - this.headers.addAll(Arrays.asList(headers));
> + synchronized (this.headers) {
> + this.headers.addAll(Arrays.asList(headers));
> + }
> }
> this.adaptHeaders = adaptHeaders;
> }
> @@ -86,7 +88,9 @@ final class Marshaller<W extends AbstractWriter<?>> {
> Map<?, ?> map = convertToMandatoryType(exchange, Map.class, row);
> if (adaptHeaders) {
> for (Object key : map.keySet()) {
> - headers.add(convertToMandatoryType(exchange, String.class, key));
> + synchronized (headers) {
> + headers.add(convertToMandatoryType(exchange, String.class, key));
> + }
> }
> }
>
>
--
Zoran Regvart