* Re: [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] <1392913353-13369-1-git-send-email-jeremie.galarneau@efficios.com>
@ 2014-02-22 16:12 ` Mathieu Desnoyers
[not found] ` <1530478584.28856.1393085579318.JavaMail.zimbra@efficios.com>
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-02-22 16:12 UTC (permalink / raw)
To: Jérémie Galarneau, David Goulet; +Cc: lttng-dev
----- Original Message -----
> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
> To: lttng-dev@lists.lttng.org
> Sent: Thursday, February 20, 2014 11:22:33 AM
> Subject: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
>
> enabledEventList is shared between the LTTngThread and eventTimer
> threads but is not synchronized.
>
> This patch changes enabledEventList's type from an ArrayList to a
> synchronizedList which implements the same interface.
>
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> ---
> .../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 15
> +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> index 25d1cb2..0d9a485 100644
> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> @@ -34,6 +34,7 @@ import java.util.List;
> import java.util.Timer;
> import java.util.TimerTask;
> import java.util.logging.Logger;
> +import java.util.Collections;
>
> class USTRegisterMsg {
> public static int pid;
> @@ -57,7 +58,8 @@ public class LTTngTCPSessiondClient {
> private Semaphore registerSem;
>
> private Timer eventTimer;
> - private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
> + private List<LTTngEvent> enabledEventList = Collections.synchronizedList(
> + new ArrayList<LTTngEvent>());
> /*
> * Map of Logger objects that have been enabled. They are indexed by name.
> */
> @@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
> * We have to make a copy here since it is possible that the
> * enabled event list is changed during an iteration on it.
> */
> - List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
> + List<LTTngEvent> tmpList;
> + synchronized(enabledEventList) {
> + tmpList = new ArrayList<LTTngEvent>(enabledEventList);
> + }
Further down in this function, we find:
ret = enableCmd.enableLogger(handler, event, enabledLoggers);
if (ret == 1) {
/* Enabled so remove the event from the list. */
enabledEventList.remove(event);
}
should we encompass a larger part of this function with synchronize ?
What happens if event is removed from enabledEventList while we iterate on the list copy ?
Thanks,
MAthieu
>
> LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
> LTTngSessiondCmd2_4.sessiond_enable_handler();
> @@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
> * Add the event to the list so it can be enabled if
> * the logger appears at some point in time.
> */
> - if (enabledEventList.contains(event) == false) {
> - enabledEventList.add(event);
> + synchronized (enabledEventList) {
> + if (enabledEventList.contains(event) == false) {
> + enabledEventList.add(event);
> + }
> }
> }
> data = enableCmd.getBytes();
> --
> 1.9.0
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] ` <1530478584.28856.1393085579318.JavaMail.zimbra@efficios.com>
@ 2014-02-22 19:15 ` Jérémie Galarneau
[not found] ` <CA+jJMxs549OwuD6QBvF6nHW8M0tHyZxcMb0oHJ6okCt4TdoY_Q@mail.gmail.com>
1 sibling, 0 replies; 8+ messages in thread
From: Jérémie Galarneau @ 2014-02-22 19:15 UTC (permalink / raw)
To: Mathieu Desnoyers; +Cc: lttng-dev
On Sat, Feb 22, 2014 at 11:12 AM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- Original Message -----
>> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
>> To: lttng-dev@lists.lttng.org
>> Sent: Thursday, February 20, 2014 11:22:33 AM
>> Subject: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
>>
>> enabledEventList is shared between the LTTngThread and eventTimer
>> threads but is not synchronized.
>>
>> This patch changes enabledEventList's type from an ArrayList to a
>> synchronizedList which implements the same interface.
>>
>> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
>> ---
>> .../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 15
>> +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> index 25d1cb2..0d9a485 100644
>> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> @@ -34,6 +34,7 @@ import java.util.List;
>> import java.util.Timer;
>> import java.util.TimerTask;
>> import java.util.logging.Logger;
>> +import java.util.Collections;
>>
>> class USTRegisterMsg {
>> public static int pid;
>> @@ -57,7 +58,8 @@ public class LTTngTCPSessiondClient {
>> private Semaphore registerSem;
>>
>> private Timer eventTimer;
>> - private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
>> + private List<LTTngEvent> enabledEventList = Collections.synchronizedList(
>> + new ArrayList<LTTngEvent>());
>> /*
>> * Map of Logger objects that have been enabled. They are indexed by name.
>> */
>> @@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
>> * We have to make a copy here since it is possible that the
>> * enabled event list is changed during an iteration on it.
>> */
>> - List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
>> + List<LTTngEvent> tmpList;
>> + synchronized(enabledEventList) {
>> + tmpList = new ArrayList<LTTngEvent>(enabledEventList);
>> + }
>
>
> Further down in this function, we find:
>
> ret = enableCmd.enableLogger(handler, event, enabledLoggers);
> if (ret == 1) {
> /* Enabled so remove the event from the list. */
> enabledEventList.remove(event);
> }
>
> should we encompass a larger part of this function with synchronize ?
AFAIU, this operation is already synchronized by the synchronizedList class.
> What happens if event is removed from enabledEventList while we iterate on the list copy ?
I'm not sure I understand your concern.
Regards,
Jérémie
>
> Thanks,
>
> MAthieu
>
>
>>
>> LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
>> LTTngSessiondCmd2_4.sessiond_enable_handler();
>> @@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
>> * Add the event to the list so it can be enabled if
>> * the logger appears at some point in time.
>> */
>> - if (enabledEventList.contains(event) == false) {
>> - enabledEventList.add(event);
>> + synchronized (enabledEventList) {
>> + if (enabledEventList.contains(event) == false) {
>> + enabledEventList.add(event);
>> + }
>> }
>> }
>> data = enableCmd.getBytes();
>> --
>> 1.9.0
>>
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev@lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] ` <CA+jJMxs549OwuD6QBvF6nHW8M0tHyZxcMb0oHJ6okCt4TdoY_Q@mail.gmail.com>
@ 2014-02-22 20:08 ` Mathieu Desnoyers
0 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-02-22 20:08 UTC (permalink / raw)
To: Jérémie Galarneau; +Cc: lttng-dev
----- Original Message -----
> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "David Goulet" <dgoulet@efficios.com>, lttng-dev@lists.lttng.org
> Sent: Saturday, February 22, 2014 2:15:15 PM
> Subject: Re: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
>
> On Sat, Feb 22, 2014 at 11:12 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> > ----- Original Message -----
> >> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
> >> To: lttng-dev@lists.lttng.org
> >> Sent: Thursday, February 20, 2014 11:22:33 AM
> >> Subject: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in
> >> LTTngTCPSessiondClient
> >>
> >> enabledEventList is shared between the LTTngThread and eventTimer
> >> threads but is not synchronized.
> >>
> >> This patch changes enabledEventList's type from an ArrayList to a
> >> synchronizedList which implements the same interface.
> >>
> >> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> >> ---
> >> .../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 15
> >> +++++++++++----
> >> 1 file changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git
> >> a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> >> b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> >> index 25d1cb2..0d9a485 100644
> >> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> >> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> >> @@ -34,6 +34,7 @@ import java.util.List;
> >> import java.util.Timer;
> >> import java.util.TimerTask;
> >> import java.util.logging.Logger;
> >> +import java.util.Collections;
> >>
> >> class USTRegisterMsg {
> >> public static int pid;
> >> @@ -57,7 +58,8 @@ public class LTTngTCPSessiondClient {
> >> private Semaphore registerSem;
> >>
> >> private Timer eventTimer;
> >> - private List<LTTngEvent> enabledEventList = new
> >> ArrayList<LTTngEvent>();
> >> + private List<LTTngEvent> enabledEventList =
> >> Collections.synchronizedList(
> >> + new ArrayList<LTTngEvent>());
> >> /*
> >> * Map of Logger objects that have been enabled. They are indexed by
> >> name.
> >> */
> >> @@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
> >> * We have to make a copy here since it is
> >> possible that the
> >> * enabled event list is changed during an
> >> iteration on it.
> >> */
> >> - List<LTTngEvent> tmpList = new
> >> ArrayList<LTTngEvent>(enabledEventList);
> >> + List<LTTngEvent> tmpList;
> >> + synchronized(enabledEventList) {
> >> + tmpList = new
> >> ArrayList<LTTngEvent>(enabledEventList);
> >> + }
> >
> >
> > Further down in this function, we find:
> >
> > ret =
> > enableCmd.enableLogger(handler,
> > event, enabledLoggers);
> > if (ret == 1) {
> > /* Enabled so remove the
> > event from the list. */
> > enabledEventList.remove(event);
> > }
> >
> > should we encompass a larger part of this function with synchronize ?
>
> AFAIU, this operation is already synchronized by the synchronizedList class.
Actually, I got that the remove operation has the synchronize
implicit. Also, it should be OK if the event is in the list,
and if it is not. Moreover, the concurrent thread only do "add",
never remove, so if it was in the list copy, it will be there for
the remove.
>
> > What happens if event is removed from enabledEventList while we iterate on
> > the list copy ?
>
> I'm not sure I understand your concern.
It's OK, see my reply above.
I'll do another reply to your patch for other comments.
Thanks,
Mathieu
>
> Regards,
> Jérémie
>
> >
> > Thanks,
> >
> > MAthieu
> >
> >
> >>
> >> LTTngSessiondCmd2_4.sessiond_enable_handler
> >> enableCmd = new
> >> LTTngSessiondCmd2_4.sessiond_enable_handler();
> >> @@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
> >> * Add the event to the list
> >> so it can be enabled if
> >> * the logger appears at
> >> some point in time.
> >> */
> >> - if
> >> (enabledEventList.contains(event) == false) {
> >> -
> >> enabledEventList.add(event);
> >> + synchronized
> >> (enabledEventList) {
> >> + if
> >> (enabledEventList.contains(event) == false) {
> >> +
> >> enabledEventList.add(event);
> >> + }
> >> }
> >> }
> >> data = enableCmd.getBytes();
> >> --
> >> 1.9.0
> >>
> >>
> >> _______________________________________________
> >> lttng-dev mailing list
> >> lttng-dev@lists.lttng.org
> >> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >>
> >
> > --
> > Mathieu Desnoyers
> > EfficiOS Inc.
> > http://www.efficios.com
>
>
>
> --
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] <1392913353-13369-1-git-send-email-jeremie.galarneau@efficios.com>
2014-02-22 16:12 ` [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient Mathieu Desnoyers
[not found] ` <1530478584.28856.1393085579318.JavaMail.zimbra@efficios.com>
@ 2014-02-22 20:11 ` Mathieu Desnoyers
[not found] ` <204474029.28925.1393099864388.JavaMail.zimbra@efficios.com>
3 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-02-22 20:11 UTC (permalink / raw)
To: Jérémie Galarneau; +Cc: lttng-dev
Other comments,
----- Original Message -----
> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
> To: lttng-dev@lists.lttng.org
> Sent: Thursday, February 20, 2014 11:22:33 AM
> Subject: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
>
> enabledEventList is shared between the LTTngThread and eventTimer
> threads but is not synchronized.
>
> This patch changes enabledEventList's type from an ArrayList to a
> synchronizedList which implements the same interface.
>
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> ---
> .../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 15
> +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> index 25d1cb2..0d9a485 100644
> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> @@ -34,6 +34,7 @@ import java.util.List;
> import java.util.Timer;
> import java.util.TimerTask;
> import java.util.logging.Logger;
> +import java.util.Collections;
>
> class USTRegisterMsg {
> public static int pid;
> @@ -57,7 +58,8 @@ public class LTTngTCPSessiondClient {
> private Semaphore registerSem;
>
> private Timer eventTimer;
> - private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
> + private List<LTTngEvent> enabledEventList = Collections.synchronizedList(
> + new ArrayList<LTTngEvent>());
I'd prefer:
private List<LTTngEvent> enabledEventList =
Collections.synchronizedList(new ArrayList<LTTngEvent>());
unless you tell me that your coding style is more "java-like".
> /*
> * Map of Logger objects that have been enabled. They are indexed by name.
> */
> @@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
> * We have to make a copy here since it is possible that the
> * enabled event list is changed during an iteration on it.
> */
> - List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
> + List<LTTngEvent> tmpList;
> + synchronized(enabledEventList) {
inconsistent style, missing space after "synchronized".
> + tmpList = new ArrayList<LTTngEvent>(enabledEventList);
> + }
>
> LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
> LTTngSessiondCmd2_4.sessiond_enable_handler();
> @@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
> * Add the event to the list so it can be enabled if
> * the logger appears at some point in time.
> */
> - if (enabledEventList.contains(event) == false) {
> - enabledEventList.add(event);
> + synchronized (enabledEventList) {
> + if (enabledEventList.contains(event) == false) {
> + enabledEventList.add(event);
> + }
Hrm, so what we want here is probably more a set than a list ? We want a data structure
that does not have duplicates. If we can confirm that the order of the elements does not
matter, a hash map might be a much more suited data structure than a list. We could
then remove the explicit synchronize around "add".
Thanks,
Mathieu
> }
> }
> data = enableCmd.getBytes();
> --
> 1.9.0
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] ` <204474029.28925.1393099864388.JavaMail.zimbra@efficios.com>
@ 2014-02-22 20:19 ` Jérémie Galarneau
2014-02-25 21:32 ` [PATCH lttng-ust v2] " Jérémie Galarneau
[not found] ` <1393363925-4773-1-git-send-email-jeremie.galarneau@efficios.com>
2 siblings, 0 replies; 8+ messages in thread
From: Jérémie Galarneau @ 2014-02-22 20:19 UTC (permalink / raw)
To: Mathieu Desnoyers; +Cc: lttng-dev
On Sat, Feb 22, 2014 at 3:11 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> Other comments,
>
> ----- Original Message -----
>> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
>> To: lttng-dev@lists.lttng.org
>> Sent: Thursday, February 20, 2014 11:22:33 AM
>> Subject: [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
>>
>> enabledEventList is shared between the LTTngThread and eventTimer
>> threads but is not synchronized.
>>
>> This patch changes enabledEventList's type from an ArrayList to a
>> synchronizedList which implements the same interface.
>>
>> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
>> ---
>> .../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 15
>> +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> index 25d1cb2..0d9a485 100644
>> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
>> @@ -34,6 +34,7 @@ import java.util.List;
>> import java.util.Timer;
>> import java.util.TimerTask;
>> import java.util.logging.Logger;
>> +import java.util.Collections;
>>
>> class USTRegisterMsg {
>> public static int pid;
>> @@ -57,7 +58,8 @@ public class LTTngTCPSessiondClient {
>> private Semaphore registerSem;
>>
>> private Timer eventTimer;
>> - private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
>> + private List<LTTngEvent> enabledEventList = Collections.synchronizedList(
>> + new ArrayList<LTTngEvent>());
>
> I'd prefer:
>
> private List<LTTngEvent> enabledEventList =
> Collections.synchronizedList(new ArrayList<LTTngEvent>());
>
> unless you tell me that your coding style is more "java-like".
>
>> /*
>> * Map of Logger objects that have been enabled. They are indexed by name.
>> */
>> @@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
>> * We have to make a copy here since it is possible that the
>> * enabled event list is changed during an iteration on it.
>> */
>> - List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
>> + List<LTTngEvent> tmpList;
>> + synchronized(enabledEventList) {
>
> inconsistent style, missing space after "synchronized".
>
>> + tmpList = new ArrayList<LTTngEvent>(enabledEventList);
>> + }
>>
>> LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
>> LTTngSessiondCmd2_4.sessiond_enable_handler();
>> @@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
>> * Add the event to the list so it can be enabled if
>> * the logger appears at some point in time.
>> */
>> - if (enabledEventList.contains(event) == false) {
>> - enabledEventList.add(event);
>> + synchronized (enabledEventList) {
>> + if (enabledEventList.contains(event) == false) {
>> + enabledEventList.add(event);
>> + }
>
> Hrm, so what we want here is probably more a set than a list ? We want a data structure
> that does not have duplicates. If we can confirm that the order of the elements does not
> matter, a hash map might be a much more suited data structure than a list. We could
> then remove the explicit synchronize around "add".
Makes sense, I guess David (original author) will probably have an
opinion on that. My immediate concern was addressing the code's thread
safety.
Regards,
Jérémie
>
> Thanks,
>
> Mathieu
>
>> }
>> }
>> data = enableCmd.getBytes();
>> --
>> 1.9.0
>>
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev@lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] ` <204474029.28925.1393099864388.JavaMail.zimbra@efficios.com>
2014-02-22 20:19 ` Jérémie Galarneau
@ 2014-02-25 21:32 ` Jérémie Galarneau
[not found] ` <1393363925-4773-1-git-send-email-jeremie.galarneau@efficios.com>
2 siblings, 0 replies; 8+ messages in thread
From: Jérémie Galarneau @ 2014-02-25 21:32 UTC (permalink / raw)
To: lttng-dev
enabledEventList is shared between the LTTngThread and eventTimer
threads but is not synchronized.
This patch changes enabledEventList's type from an ArrayList to a
synchronized HashSet.
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---
.../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 129 ++++++++++++---------
1 file changed, 74 insertions(+), 55 deletions(-)
diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
index 25d1cb2..35f768f 100644
--- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
+++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
@@ -30,10 +30,14 @@ import java.net.*;
import java.lang.management.ManagementFactory;
import java.util.ArrayList;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
import java.util.List;
+import java.util.Set;
import java.util.Timer;
import java.util.TimerTask;
import java.util.logging.Logger;
+import java.util.Collections;
class USTRegisterMsg {
public static int pid;
@@ -57,7 +61,8 @@ public class LTTngTCPSessiondClient {
private Semaphore registerSem;
private Timer eventTimer;
- private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
+ private Set<LTTngEvent> enabledEventSet =
+ Collections.synchronizedSet(new HashSet<LTTngEvent>());
/*
* Map of Logger objects that have been enabled. They are indexed by name.
*/
@@ -82,65 +87,81 @@ public class LTTngTCPSessiondClient {
this.eventTimer.scheduleAtFixedRate(new TimerTask() {
@Override
public void run() {
- /*
- * We have to make a copy here since it is possible that the
- * enabled event list is changed during an iteration on it.
- */
- List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
-
- LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
- LTTngSessiondCmd2_4.sessiond_enable_handler();
- for (LTTngEvent event: tmpList) {
- int ret;
- Logger logger;
-
+ synchronized (enabledEventSet) {
+ LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
+ LTTngSessiondCmd2_4.sessiond_enable_handler();
/*
- * Check if this Logger name has been enabled already. Note
- * that in the case of "*", it's never added in that hash
- * table thus the enable command does a lookup for each
- * logger name in that hash table for the * case in order
- * to make sure we don't enable twice the same logger
- * because JUL apparently accepts that the *same*
- * LogHandler can be added twice on a Logger object...
- * don't ask...
+ * Modifying events in a Set will raise a
+ * ConcurrentModificationException. Thus, we remove an event
+ * and add its modified version to modifiedEvents when a
+ * modification is necessary.
*/
- logger = enabledLoggers.get(event.name);
- if (logger != null) {
- continue;
- }
+ Set<LTTngEvent> modifiedEvents = new HashSet<LTTngEvent>();
+ Iterator<LTTngEvent> it = enabledEventSet.iterator();
- /*
- * Set to one means that the enable all event has been seen
- * thus event from that point on must use loglevel for all
- * events. Else the object has its own loglevel.
- */
- if (handler.logLevelUseAll == 1) {
- event.logLevel.level = handler.logLevelAll;
- event.logLevel.type = handler.logLevelTypeAll;
- }
+ while (it.hasNext()) {
+ int ret;
+ Logger logger;
+ LTTngEvent event = it.next();
- /*
- * The all event is a special case since we have to iterate
- * over every Logger to see which one was not enabled.
- */
- if (event.name.equals("*")) {
- enableCmd.name = event.name;
- enableCmd.lttngLogLevel = event.logLevel.level;
- enableCmd.lttngLogLevelType = event.logLevel.type;
/*
- * The return value is irrelevant since the * event is
- * always kept in the list.
+ * Check if this Logger name has been enabled already. Note
+ * that in the case of "*", it's never added in that hash
+ * table thus the enable command does a lookup for each
+ * logger name in that hash table for the * case in order
+ * to make sure we don't enable twice the same logger
+ * because JUL apparently accepts that the *same*
+ * LogHandler can be added twice on a Logger object...
+ * don't ask...
*/
- enableCmd.execute(handler, enabledLoggers);
- continue;
- }
+ logger = enabledLoggers.get(event.name);
+ if (logger != null) {
+ continue;
+ }
- ret = enableCmd.enableLogger(handler, event, enabledLoggers);
- if (ret == 1) {
- /* Enabled so remove the event from the list. */
- enabledEventList.remove(event);
+ /*
+ * Set to one means that the enable all event has been seen
+ * thus event from that point on must use loglevel for all
+ * events. Else the object has its own loglevel.
+ */
+ if (handler.logLevelUseAll == 1) {
+ it.remove();
+ event.logLevel.level = handler.logLevelAll;
+ event.logLevel.type = handler.logLevelTypeAll;
+ modifiedEvents.add(event);
+ }
+
+ /*
+ * The all event is a special case since we have to iterate
+ * over every Logger to see which one was not enabled.
+ */
+ if (event.name.equals("*")) {
+ enableCmd.name = event.name;
+ enableCmd.lttngLogLevel = event.logLevel.level;
+ enableCmd.lttngLogLevelType = event.logLevel.type;
+ /*
+ * The return value is irrelevant since the * event is
+ * always kept in the set.
+ */
+ enableCmd.execute(handler, enabledLoggers);
+ continue;
+ }
+
+ ret = enableCmd.enableLogger(handler, event, enabledLoggers);
+ if (ret == 1) {
+ /* Enabled so remove the event from the set. */
+ if (!modifiedEvents.remove(event)) {
+ /*
+ * event can only be present in one of
+ * the sets.
+ */
+ it.remove();
+ }
+ }
}
+ enabledEventSet.addAll(modifiedEvents);
}
+
}
}, this.timerDelay, this.timerDelay);
@@ -274,12 +295,10 @@ public class LTTngTCPSessiondClient {
event = enableCmd.execute(this.handler, this.enabledLoggers);
if (event != null) {
/*
- * Add the event to the list so it can be enabled if
+ * Add the event to the set so it can be enabled if
* the logger appears at some point in time.
*/
- if (enabledEventList.contains(event) == false) {
- enabledEventList.add(event);
- }
+ enabledEventSet.add(event);
}
data = enableCmd.getBytes();
break;
--
1.9.0
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient
[not found] ` <1393363925-4773-1-git-send-email-jeremie.galarneau@efficios.com>
@ 2014-02-25 23:01 ` Mathieu Desnoyers
0 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-02-25 23:01 UTC (permalink / raw)
To: Jérémie Galarneau; +Cc: lttng-dev
merged, thanks!
Mathieu
----- Original Message -----
> From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
> To: lttng-dev@lists.lttng.org
> Sent: Tuesday, February 25, 2014 4:32:05 PM
> Subject: [lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient
>
> enabledEventList is shared between the LTTngThread and eventTimer
> threads but is not synchronized.
>
> This patch changes enabledEventList's type from an ArrayList to a
> synchronized HashSet.
>
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> ---
> .../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 129
> ++++++++++++---------
> 1 file changed, 74 insertions(+), 55 deletions(-)
>
> diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> index 25d1cb2..35f768f 100644
> --- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> +++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
> @@ -30,10 +30,14 @@ import java.net.*;
> import java.lang.management.ManagementFactory;
> import java.util.ArrayList;
> import java.util.HashMap;
> +import java.util.HashSet;
> +import java.util.Iterator;
> import java.util.List;
> +import java.util.Set;
> import java.util.Timer;
> import java.util.TimerTask;
> import java.util.logging.Logger;
> +import java.util.Collections;
>
> class USTRegisterMsg {
> public static int pid;
> @@ -57,7 +61,8 @@ public class LTTngTCPSessiondClient {
> private Semaphore registerSem;
>
> private Timer eventTimer;
> - private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
> + private Set<LTTngEvent> enabledEventSet =
> + Collections.synchronizedSet(new HashSet<LTTngEvent>());
> /*
> * Map of Logger objects that have been enabled. They are indexed by name.
> */
> @@ -82,65 +87,81 @@ public class LTTngTCPSessiondClient {
> this.eventTimer.scheduleAtFixedRate(new TimerTask() {
> @Override
> public void run() {
> - /*
> - * We have to make a copy here since it is possible that the
> - * enabled event list is changed during an iteration on it.
> - */
> - List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
> -
> - LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
> - LTTngSessiondCmd2_4.sessiond_enable_handler();
> - for (LTTngEvent event: tmpList) {
> - int ret;
> - Logger logger;
> -
> + synchronized (enabledEventSet) {
> + LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
> + LTTngSessiondCmd2_4.sessiond_enable_handler();
> /*
> - * Check if this Logger name has been enabled already. Note
> - * that in the case of "*", it's never added in that hash
> - * table thus the enable command does a lookup for each
> - * logger name in that hash table for the * case in order
> - * to make sure we don't enable twice the same logger
> - * because JUL apparently accepts that the *same*
> - * LogHandler can be added twice on a Logger object...
> - * don't ask...
> + * Modifying events in a Set will raise a
> + * ConcurrentModificationException. Thus, we remove an event
> + * and add its modified version to modifiedEvents when a
> + * modification is necessary.
> */
> - logger = enabledLoggers.get(event.name);
> - if (logger != null) {
> - continue;
> - }
> + Set<LTTngEvent> modifiedEvents = new HashSet<LTTngEvent>();
> + Iterator<LTTngEvent> it = enabledEventSet.iterator();
>
> - /*
> - * Set to one means that the enable all event has been seen
> - * thus event from that point on must use loglevel for all
> - * events. Else the object has its own loglevel.
> - */
> - if (handler.logLevelUseAll == 1) {
> - event.logLevel.level = handler.logLevelAll;
> - event.logLevel.type = handler.logLevelTypeAll;
> - }
> + while (it.hasNext()) {
> + int ret;
> + Logger logger;
> + LTTngEvent event = it.next();
>
> - /*
> - * The all event is a special case since we have to iterate
> - * over every Logger to see which one was not enabled.
> - */
> - if (event.name.equals("*")) {
> - enableCmd.name = event.name;
> - enableCmd.lttngLogLevel = event.logLevel.level;
> - enableCmd.lttngLogLevelType = event.logLevel.type;
> /*
> - * The return value is irrelevant since the * event is
> - * always kept in the list.
> + * Check if this Logger name has been enabled already. Note
> + * that in the case of "*", it's never added in that hash
> + * table thus the enable command does a lookup for each
> + * logger name in that hash table for the * case in order
> + * to make sure we don't enable twice the same logger
> + * because JUL apparently accepts that the *same*
> + * LogHandler can be added twice on a Logger object...
> + * don't ask...
> */
> - enableCmd.execute(handler, enabledLoggers);
> - continue;
> - }
> + logger = enabledLoggers.get(event.name);
> + if (logger != null) {
> + continue;
> + }
>
> - ret = enableCmd.enableLogger(handler, event, enabledLoggers);
> - if (ret == 1) {
> - /* Enabled so remove the event from the list. */
> - enabledEventList.remove(event);
> + /*
> + * Set to one means that the enable all event has been seen
> + * thus event from that point on must use loglevel for all
> + * events. Else the object has its own loglevel.
> + */
> + if (handler.logLevelUseAll == 1) {
> + it.remove();
> + event.logLevel.level = handler.logLevelAll;
> + event.logLevel.type = handler.logLevelTypeAll;
> + modifiedEvents.add(event);
> + }
> +
> + /*
> + * The all event is a special case since we have to iterate
> + * over every Logger to see which one was not enabled.
> + */
> + if (event.name.equals("*")) {
> + enableCmd.name = event.name;
> + enableCmd.lttngLogLevel = event.logLevel.level;
> + enableCmd.lttngLogLevelType = event.logLevel.type;
> + /*
> + * The return value is irrelevant since the * event is
> + * always kept in the set.
> + */
> + enableCmd.execute(handler, enabledLoggers);
> + continue;
> + }
> +
> + ret = enableCmd.enableLogger(handler, event, enabledLoggers);
> + if (ret == 1) {
> + /* Enabled so remove the event from the set. */
> + if (!modifiedEvents.remove(event)) {
> + /*
> + * event can only be present in one of
> + * the sets.
> + */
> + it.remove();
> + }
> + }
> }
> + enabledEventSet.addAll(modifiedEvents);
> }
> +
> }
> }, this.timerDelay, this.timerDelay);
>
> @@ -274,12 +295,10 @@ public class LTTngTCPSessiondClient {
> event = enableCmd.execute(this.handler, this.enabledLoggers);
> if (event != null) {
> /*
> - * Add the event to the list so it can be enabled if
> + * Add the event to the set so it can be enabled if
> * the logger appears at some point in time.
> */
> - if (enabledEventList.contains(event) == false) {
> - enabledEventList.add(event);
> - }
> + enabledEventSet.add(event);
> }
> data = enableCmd.getBytes();
> break;
> --
> 1.9.0
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient
@ 2014-02-20 16:22 Jérémie Galarneau
0 siblings, 0 replies; 8+ messages in thread
From: Jérémie Galarneau @ 2014-02-20 16:22 UTC (permalink / raw)
To: lttng-dev
enabledEventList is shared between the LTTngThread and eventTimer
threads but is not synchronized.
This patch changes enabledEventList's type from an ArrayList to a
synchronizedList which implements the same interface.
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---
.../org/lttng/ust/jul/LTTngTCPSessiondClient.java | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
index 25d1cb2..0d9a485 100644
--- a/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
+++ b/liblttng-ust-jul/org/lttng/ust/jul/LTTngTCPSessiondClient.java
@@ -34,6 +34,7 @@ import java.util.List;
import java.util.Timer;
import java.util.TimerTask;
import java.util.logging.Logger;
+import java.util.Collections;
class USTRegisterMsg {
public static int pid;
@@ -57,7 +58,8 @@ public class LTTngTCPSessiondClient {
private Semaphore registerSem;
private Timer eventTimer;
- private List<LTTngEvent> enabledEventList = new ArrayList<LTTngEvent>();
+ private List<LTTngEvent> enabledEventList = Collections.synchronizedList(
+ new ArrayList<LTTngEvent>());
/*
* Map of Logger objects that have been enabled. They are indexed by name.
*/
@@ -86,7 +88,10 @@ public class LTTngTCPSessiondClient {
* We have to make a copy here since it is possible that the
* enabled event list is changed during an iteration on it.
*/
- List<LTTngEvent> tmpList = new ArrayList<LTTngEvent>(enabledEventList);
+ List<LTTngEvent> tmpList;
+ synchronized(enabledEventList) {
+ tmpList = new ArrayList<LTTngEvent>(enabledEventList);
+ }
LTTngSessiondCmd2_4.sessiond_enable_handler enableCmd = new
LTTngSessiondCmd2_4.sessiond_enable_handler();
@@ -277,8 +282,10 @@ public class LTTngTCPSessiondClient {
* Add the event to the list so it can be enabled if
* the logger appears at some point in time.
*/
- if (enabledEventList.contains(event) == false) {
- enabledEventList.add(event);
+ synchronized (enabledEventList) {
+ if (enabledEventList.contains(event) == false) {
+ enabledEventList.add(event);
+ }
}
}
data = enableCmd.getBytes();
--
1.9.0
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-25 23:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1392913353-13369-1-git-send-email-jeremie.galarneau@efficios.com>
2014-02-22 16:12 ` [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient Mathieu Desnoyers
[not found] ` <1530478584.28856.1393085579318.JavaMail.zimbra@efficios.com>
2014-02-22 19:15 ` Jérémie Galarneau
[not found] ` <CA+jJMxs549OwuD6QBvF6nHW8M0tHyZxcMb0oHJ6okCt4TdoY_Q@mail.gmail.com>
2014-02-22 20:08 ` Mathieu Desnoyers
2014-02-22 20:11 ` Mathieu Desnoyers
[not found] ` <204474029.28925.1393099864388.JavaMail.zimbra@efficios.com>
2014-02-22 20:19 ` Jérémie Galarneau
2014-02-25 21:32 ` [PATCH lttng-ust v2] " Jérémie Galarneau
[not found] ` <1393363925-4773-1-git-send-email-jeremie.galarneau@efficios.com>
2014-02-25 23:01 ` Mathieu Desnoyers
2014-02-20 16:22 [PATCH lttng-ust] " Jérémie Galarneau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.