* [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient @ 2014-02-20 16:22 Jérémie Galarneau 2014-02-22 16:12 ` Mathieu Desnoyers 2014-02-22 20:11 ` Mathieu Desnoyers 0 siblings, 2 replies; 8+ messages in thread From: Jérémie Galarneau @ 2014-02-20 16:22 UTC (permalink / raw) 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 at 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-20 16:22 [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient Jérémie Galarneau @ 2014-02-22 16:12 ` Mathieu Desnoyers 2014-02-22 19:15 ` Jérémie Galarneau 2014-02-22 20:11 ` Mathieu Desnoyers 1 sibling, 1 reply; 8+ messages in thread From: Mathieu Desnoyers @ 2014-02-22 16:12 UTC (permalink / raw) ----- Original Message ----- > From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> > To: lttng-dev at 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 at 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 at lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-22 16:12 ` Mathieu Desnoyers @ 2014-02-22 19:15 ` Jérémie Galarneau 2014-02-22 20:08 ` Mathieu Desnoyers 0 siblings, 1 reply; 8+ messages in thread From: Jérémie Galarneau @ 2014-02-22 19:15 UTC (permalink / raw) On Sat, Feb 22, 2014 at 11:12 AM, Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote: > ----- Original Message ----- >> From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> >> To: lttng-dev at 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 at 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 at 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
* [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-22 19:15 ` Jérémie Galarneau @ 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) ----- Original Message ----- > From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com> > Cc: "David Goulet" <dgoulet at efficios.com>, lttng-dev at 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 at efficios.com> wrote: > > ----- Original Message ----- > >> From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> > >> To: lttng-dev at 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 at 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 at 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-20 16:22 [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient Jérémie Galarneau 2014-02-22 16:12 ` Mathieu Desnoyers @ 2014-02-22 20:11 ` Mathieu Desnoyers 2014-02-22 20:19 ` Jérémie Galarneau 2014-02-25 21:32 ` [lttng-dev] [PATCH lttng-ust v2] " Jérémie Galarneau 1 sibling, 2 replies; 8+ messages in thread From: Mathieu Desnoyers @ 2014-02-22 20:11 UTC (permalink / raw) Other comments, ----- Original Message ----- > From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> > To: lttng-dev at 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 at 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 at lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-22 20:11 ` Mathieu Desnoyers @ 2014-02-22 20:19 ` Jérémie Galarneau 2014-02-25 21:32 ` [lttng-dev] [PATCH lttng-ust v2] " Jérémie Galarneau 1 sibling, 0 replies; 8+ messages in thread From: Jérémie Galarneau @ 2014-02-22 20:19 UTC (permalink / raw) On Sat, Feb 22, 2014 at 3:11 PM, Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote: > Other comments, > > ----- Original Message ----- >> From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> >> To: lttng-dev at 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 at 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 at 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
* [lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-22 20:11 ` Mathieu Desnoyers 2014-02-22 20:19 ` Jérémie Galarneau @ 2014-02-25 21:32 ` Jérémie Galarneau 2014-02-25 23:01 ` Mathieu Desnoyers 1 sibling, 1 reply; 8+ messages in thread From: Jérémie Galarneau @ 2014-02-25 21:32 UTC (permalink / raw) 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 at 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lttng-dev] [PATCH lttng-ust v2] Fix: Unsynchronized access in LTTngTCPSessiondClient 2014-02-25 21:32 ` [lttng-dev] [PATCH lttng-ust v2] " Jérémie Galarneau @ 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) merged, thanks! Mathieu ----- Original Message ----- > From: "J?r?mie Galarneau" <jeremie.galarneau@efficios.com> > To: lttng-dev at 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 at 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 at lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [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 -- 2014-02-20 16:22 [lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient Jérémie Galarneau 2014-02-22 16:12 ` Mathieu Desnoyers 2014-02-22 19:15 ` Jérémie Galarneau 2014-02-22 20:08 ` Mathieu Desnoyers 2014-02-22 20:11 ` Mathieu Desnoyers 2014-02-22 20:19 ` Jérémie Galarneau 2014-02-25 21:32 ` [lttng-dev] [PATCH lttng-ust v2] " Jérémie Galarneau 2014-02-25 23:01 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox