6 ms·
How can anyone program sanely in the presence of this: currentPos = new Point(currentPos.x+1, currentPos.y+1); does a few things, including writing default val
by boothead 13y ago
How can anyone program sanely in the presence of this:
currentPos = new Point(currentPos.x+1, currentPos.y+1); does a few things, including writing default values to x and y (0) and then writing their initial values in the constructor. Since your object is not safely published those 4 write operations can be freely reordered by the compiler / JVM.
So from the perspective of the reading thread, it is a legal execution to read x with its new value but y with its default value of 0 for example. By the time you reach the println statement (which by the way is synchronized and therefore does influence the read operations), the variables have their initial values and the program prints the expected values.
I'm not anywhere near smart or careful enough for that... I think I'll stick with Haskell.
- jlarocco 13y agoHonestly I don't think Haskell would have helped much here. That code is terrible from a thread safety perspective. If the author didn't read up on how to write good thread-safe code, it's unlikely he'd read up on Haskell, so he'd probably write bad code there, too.
- lelf 13y agoHaskell has type-safe software transactional memory for this stuff. And it's exponentially harder to get a program that compiles and has error like that.
- jlarocco 13y agoThe author didn't take the time to learn the very basics of thread safety. It's highly unlikely he would take the time to learn Haskell and software transactional memory.
- conroe64 13y agoCan you explain what makes it so terrible? At first glance it seems as if it is thread safe, and avoiding synchronization locks can provide a nice speed up. The notion that currentPos could be set to the new Point before the Point(int x, int y) constructor completed was completely surprising to me. Is that something that was obvious to you, or do you have other reasons to dislike it?
- hexagonc 13y agoYeah, this is very surprising to me. I simply didn't know that java worked that way and I've been programming in java for many years. I assumed that the memory that "currentPos" would eventually be bound to was fully initialized before the assignment operator remapped "currentPos" to the new value. I also assumed that binding a variable name to a new value is atomic across all threads.
- jlarocco 13y agocurrentPos is modified in one thread and read in a different thread, outside of a synchronized block, and without being protected by a mutex. It's a pretty straightforward recipe for disaster.
- mikeash 13y agoThe One Commandment of multithreaded programming: 1. Thou Shalt Not touch shared data without synchronization. You must know what data is shared, and you must synchronize all access to shared data (with one exception, when all access is read-only). People try to find exceptions to this One Commandment. They think, I'm only writing one value, so what's the harm? Well, this is the harm. Follow the One Commandment and you'll be safe(r).
- awj 13y agoNo real argument here, but Haskell does encourage you to use read only data structures unless you truly need writes. That said, there's still a wide variety of clever ways in which you can find your way to multithreading hell.
- jtheory 13y agoRead-only data structures are a common idiom in Java as well (the classic example is String), and do indeed help reduce the headaches.
- windust 13y agoNot sure this is true. There is the concept of final fields, but that doesn't make objects immutable, and thus even when declaring object finals (or arrays) you can pretty easily shoot yourself in the foot by mutating them in-place.
- mbucc 13y agoFor immutable collections, guava is helpful.
- jtheory 13y agoIt is -- but you still do have to be aware if the objects in your collection are also immutable... if so, you may end up with an "immutable" list of 5 objects, and down the line it will still have those 5 objects, but the objects themselves may have completely different internal data. Referring partly to your parent comment... it's instructive to look at the String implementation. It's immutable, but they have to jump through a few hoops to do it properly... internally, a String has a private char array. An array is not immutable, so the code has to be very careful to never expose that char array externally. Because of that, calling toCharArray() will copy the internal array and return the copy -- that one's fairly obvious. But less obviously, a String constructed with a char array must also make a copy... otherwise the calling code might then modify the array after creating the String with it. So sure, immutable objects are possible, and widely encouraged in Java, but they're easy to do wrongly/incompletely.
- jbooth 13y agoinsert "dumb code in any language" rant here
- matzipan 13y agoGet real
- jtheory 13y agoYou don't have to know the majority of that, though. These are tricks the compiler/VM can use, because (unless you say otherwise in code) it's assumed that there's no sharing data between threads. Most programmers (regardless of the language) just learn the rules about programming with threads -- they don't care why exactly following the rules is important.
- jbn 13y agoSure those programmers learn the rules... then they promptly forget them because they seem too complicated and "it works on my machine with only one client".. I agree with you, one must know why rules are important.. but in large systems, it's better if the system/language enforces this separation of data (praise Erlang).
- xsmasher 13y agoIn less technical language, "creation of the object is not atomic". You could switch threads in the middle of the creation. Also, "reading of the data is not atomic" - you can switch threads in the middle of reading the data. In order to program THREADED CODE sanely with that in mind, you need to minimize the amount of shared data, and be sure to lock / synchronize around the writing AND READING of any shared data. If you're not writing threaded code then there is no shared data, and you don't care about these rules. Writing threaded code isn't like dusting crops, but it's not impossible. EDIT: you also said "the println statement [...] is synchronized" but it doesn't look like that's true - there's nothing in the synchronized(this) {} block, and even if there was, all it would do is keep "this" from entering that block twice; it has no effect on other threads in this code.
- Eiwatah4 13y agoYou misunderstood the part about println. The implementation of println() uses synchronized internally. You can't see it without downloading the Java source code, but it's there.
- xsmasher 13y agoAh, thanks. I don't do much Java. So "println() uses synchronized internally" means two println statements will never overlap each other, but still says nothing about their interaction with other objects, right? Does nothing to prevent switching threads in the middle of the println and getting two different values?
- ddeck 13y agoCorrect. It synchronizes on the instance of the PrintStream, which has no effect here. The bottom line is that the Point object should have been immutable, which would have made it safe for publication. That requires its fields to be final (among other things), which was not the case.
- Someone 13y agoIn general, you shouldn't assume that tomorrow's implementation behaves like today's, or today's implementation X behaves like today's implementation Y, unless there is documentation that says so. Looking at http://stackoverflow.com/questions/9459657/synchronization-and-system-out-println http://stackoverflow.com/questions/9459657/synchronization-a..., it seems that Java does not guarantee that println will synchronize. The exception is if you have tight control over your environment, have tests in place to verify that environment upgrades do not break your assumptions, and you desperately need to bend the rules, but even then, you should look really hard for better alternatives.
- casperc 13y agoI don't program much in java, but I would have thought that the problem was in the assignment of the new Point to currentPost, not the construction of the new Point. I would have thought that the order of operations would be: 1) Read value from currentPos.x and add 1. Do same for y. 2) Pass values to constructor 3) Construct the new object 3a) init values with 0 3b) assign new values to x and y 4) Assign the newly constructed object's pointer to currentPos. I thought that the problem lay in 4 where you might read a partial update of the pointer, possibly giving weird results. Is he saying that it might get assigned first and then constructed afterwards?
- windust 13y agoYou're assuming that the code is executing in the way you read it, which is false. With the advent of branch prediction and Instruction reordering in CPUS (to gain performance), a CPU have the liberty of reordering operations for efficiency (http://en.wikipedia.org/wiki/Out-of-order_execution http://en.wikipedia.org/wiki/Out-of-order_execution) EXCEPT when there are memory barriers (or explicit synchronization instructions). With a multi-core processor things get even more complex, as you have cache locality (a thread reading the point value might be hitting the CPU cache and not main memory). If the thread happens to be executing on a different core than the assigning thread, disaster ensues.
- EdiX 13y ago> I thought that the problem lay in 4 where you might read a partial update of the pointer, possibly giving weird results. Is he saying that it might get assigned first and then constructed afterwards? Yes. In some versions of java the code: x = new X() results in assigning to x a reference to a new uninitialized object X and then a call to the constructor. Reference assignment in java is always atomic, it is guaranteed by the memory model.
- barrkel 13y agoNo. x = new X() should not make x assigned such that it is visible inside the constructor of X. The behaviour in the post can only been seen with concurrency. The issue here is the lack of a write barrier at the point of publishing the reference to the newly constructed object, and the lack of a read barrier at the point of reading the reference to the newly constructed object. You want to stop both writes moving forward in time (the writes in the constructor happening after the write that publishes the object) and reads moving backwards in time (the reads on the other thread reading the old values of the constructed object, rather than the initialized values - may be caused by e.g. satisfying the read from per-CPU cache). Java's volatile acts both ways; writes are write barriers and reads are read barriers.