On Sat, Feb 22, 2014 at 3:11 PM, Mathieu Desnoyers <[email protected]> wrote: > Other comments, > > ----- Original Message ----- >> From: "Jérémie Galarneau" <[email protected]> >> To: [email protected] >> 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 <[email protected]> >> --- >> .../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 >> [email protected] >> 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 _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
