Fork me on GitHub
#clojure-dev
<
2019-09-05
>
andy.fingerhut20:09:21

Sorry if I sound like a broken record here -- new question from me but still have Java memory model correct synchronization of data between threads bouncing in my brain, and looked at the implementations of all of the transducers within Clojure. A bunch have no internal state, so look perfectly safe to move between threads, and a bunch more have a single item of internal state created and updated using volatile!, so also look safe to me.

andy.fingerhut20:09:14

partition-by and partition-all look unsafe to me, from what I know. They have not only a volatile! in their internal state, but also a java.util.ArrayList, whose Java API docs say require outside synchronization if you do add method calls on them, which those transducers do.

andy.fingerhut20:09:40

Those java.util.ArrayList objects are not synchronized in the code of partitional-all or partition-by.

Alex Miller (Clojure team)20:09:59

there is a ticket for the partition ones

Alex Miller (Clojure team)21:09:09

if you patch, I'd be happy to screen it, it's been vetted by rich already

andy.fingerhut21:09:47

Just went looking for an existing JIRA, and then noticed you already linked to it when I came back 🙂

andy.fingerhut21:09:43

Thinking about a patch -- quickly realizing that detecting that code might be improperly synchronized, and finding the most efficient correction, are different levels of difficulty.

Alex Miller (Clojure team)21:09:06

you can follow the path of other stateful transducers using volatile

Alex Miller (Clojure team)21:09:18

I don't think it's any different than those

andy.fingerhut21:09:02

Yep, that is one way to skin that cat -- just will look a bit messy probably since there are a fair number of Java method calls for mutating it.

hiredman21:09:22

I am trying to work out if any of the current ways we have to use transducers (in core an core.async) would have an issue there, and I can't see that they would.

hiredman21:09:16

in core things don't just change threads, and in core.async thread changes all happen around taking channel locks

Alex Miller (Clojure team)22:09:29

@andy.fingerhut not sure I understand what’s hard

andy.fingerhut22:09:06

nothing hard, I believe. Except I am finding sometimes, getting attachments onto JIRA tickets.

Alex Miller (Clojure team)22:09:52

Yeah, make sure to turn off the “new” ui if you haven’t as it is bad

Alex Miller (Clojure team)22:09:59

Given the optimizations around clearing young objects, I wonder if it would actually be better to use nil for empty partition and just throw it away when done rather than clearing and reusing

Alex Miller (Clojure team)22:09:26

Would simplify the code a lot

andy.fingerhut22:09:54

I can try a variant like that.

andy.fingerhut22:09:40

clearing young objects, meaning generational GC? Or some other optimizations you are referring to there?

Alex Miller (Clojure team)22:09:46

I’m in the car so can’t properly look at the patch but doing multiple ops on the volatile has safe publication but not thread safety, but I guess that doesn’t matter in this context

andy.fingerhut22:09:39

If by thread safety you mean that things can go badly if multiple threads simultaneously call this transducer function, then perfectly agreed, but good that none of the stateful transducers are intended to work in such an environment.

andy.fingerhut23:09:03

@hiredman I would be curious to learn what you find out there.