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".
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
_______________________________________________
lttng-dev mailing list
[email protected]
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
