No worries then. Thanks! 2015-03-10 2:23 GMT+01:00 Alex Miller <[email protected]>:
> Hi Sébastien, > > Most publication in LazyTransformer is handled via the synchronized seq() > method. However, LazyTransformer is being replaced as part of > http://dev.clojure.org/jira/browse/CLJ-1669 so probably moot. > > Alex > > > > On Monday, March 9, 2015 at 5:40:11 PM UTC-5, Sébastien Bocq wrote: >> >> Hello, >> >> I noticed that LazyTransformer [1] implements mutable state without any >> volatile or final modifier to guard its fields (it might not be an isolated >> case in the core library e.g. SeqIterator.java). >> >> Isn't such class subject to unsafe publication? >> >> >> For example, considering only this part of the implementation: >> >> public final class LazyTransformer extends ... { >> >> IStepper stepper; >> Object first = null; >> LazyTransformer rest = null; >> >> public LazyTransformer(IStepper stepper){ >> this.stepper = stepper; >> } >> >> public static LazyTransformer create(IFn xform, Object coll){ >> return new LazyTransformer(new Stepper(xform, RT.iter(coll))); >> } >> ... >> >> public Object first(){ >> if(stepper != null) >> seq(); >> if(rest == null) >> return null; >> return first; >> } >> >> public ISeq next(){ >> if(stepper != null) >> seq(); >> if(rest == null) >> return null; >> return rest.seq(); >> } >> .. >> } >> >> If LazyTransformer/create is called in in one thread and then another >> thread calls 'first' or 'next' on the new object then I would say there's a >> chance the second thread sees stepper == null. A real use case could be one >> thread calling (dedup) [2], which creates lazy sequence with (sequence >> <http://crossclj.info/ns/org.clojure/clojure/1.7.0-alpha5/clojure.core.html#_sequence> >> (dedupe >> <http://crossclj.info/ns/org.clojure/clojure/1.7.0-alpha5/clojure.core.html#_dedupe>) >> coll), and then another thread iterates through the sequence, which I >> suppose relies on 'first' and 'next'. >> >> To me it looks like the exact same situation as the one discussed on the >> Java concurrency-interest here: >> http://jsr166-concurrency.10961.n7.nabble.com/Safe- >> publishing-strategy-tt12126.html >> >> I can't believe I'm the first to be worried by this so maybe I overlooked >> something. But if I'm not wrong or if there is any doubt, next to >> refactoring the code to use immutable classes with only final fields (my >> preferred approach), wouldn't it be safer to declare at least 'stepper' as >> volatile to prevent any of risk of nasty concurrency issue? >> >> Thanks, >> Sébastien >> >> [1] https://github.com/clojure/clojure/blob/master/src/jvm/ >> clojure/lang/LazyTransformer.java >> [2] http://crossclj.info/fun/clojure.core/dedupe.html >> > -- > You received this message because you are subscribed to the Google > Groups "Clojure" group. > To post to this group, send email to [email protected] > Note that posts from new members are moderated - please be patient with > your first post. > To unsubscribe from this group, send email to > [email protected] > For more options, visit this group at > http://groups.google.com/group/clojure?hl=en > --- > You received this message because you are subscribed to a topic in the > Google Groups "Clojure" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/clojure/rJiR_Dz9hX8/unsubscribe. > To unsubscribe from this group and all its topics, send an email to > [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- Sébastien -- You received this message because you are subscribed to the Google Groups "Clojure" group. To post to this group, send email to [email protected] Note that posts from new members are moderated - please be patient with your first post. To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/clojure?hl=en --- You received this message because you are subscribed to the Google Groups "Clojure" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
