[lttng-dev] [PATCH lttng-ust] Fix: Unsynchronized access in LTTngTCPSessiondClient

[jeremie.galarneau@efficios.com (Jérémie Galarneau)]

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