12

Is it safe to use Thread's methods like setName / getName and some others from different threads? API does not say anything, but judging by the source code

private char name[];

public final void setName(String name) {
    checkAccess();
    this.name = name.toCharArray();
}

public final String getName() {
    return String.valueOf(name);
}

it seems that it may cause memory consistency errors.

yair
  • 8,945
  • 4
  • 31
  • 50
Evgeniy Dorofeev
  • 133,369
  • 30
  • 199
  • 275
  • 4
    Well, if the API says nothing, then it's safe to assume they're not thread-safe. That said, is it even possible for a thread to be interrupted in the middle of a simple field read / write? I'm not sure what sort of inconsistency would occur here, since the array is never modified in-place. – millimoose Mar 14 '13 at 20:52
  • Those may not be the actual code, it could be that the methods are actually native and implemented with intrinsics. – Andrew Mao Mar 14 '13 at 20:55
  • 1
    It has to be synchronized to be thead-safe. Do you know what does `checkAccess()` method do? – Bhesh Gurung Mar 14 '13 at 20:55
  • @BheshGurung That doesn't make sense. `synchronized` and "thread-safe" are two different concepts and neither implies the other. And whatever `checkAccess()` does it can't really guarantee "thread-safety" (whatever that means) of code outside of the method call. – millimoose Mar 14 '13 at 21:01
  • Some day your code could execute using a Java standard library implementation other than OpenJDK's. Or time could pass between when `getName` (ultimately `System.arrayCopy`) copies the contents of `name` and when `getName` returns a value. During that time, `setName` could change the value of `name`, thus causing `getName` to return a value that is no longer accurate, that is, consistent. – minopret Mar 14 '13 at 21:02
  • 2
    Scratching my head why the name is stored as char[] ? Cant think of a good reason except to save a few bytes storage... or maybe it better interfaces with native code parts. – Gyro Gearless Mar 14 '13 at 21:04
  • Should be the other way round. APIs on a multithreaded, premeptive system should be threadsafe unless otherwise indicated. – Martin James Mar 14 '13 at 21:07
  • 2
    @Gyro Gearless http://stackoverflow.com/questions/10412408/why-setname-in-thread-class-assigns-to-a-character-arraywhy-not-a-string – Evgeniy Dorofeev Mar 14 '13 at 21:18
  • Umm, I'm away from compiler now... can you actually change thread name once it's started? – Denis Tulskiy Mar 14 '13 at 21:18
  • @minopret It's pretty impossible for `Thread` to make consecutive calls to its methods Do The Right Thing w/r/t desired (i.e. "thread-safe" behaviour). It doesn't really make sense to consider that scenario when talking about the class, that's up to the client to handle. Saying that a given isolated method is "thread-safe" makes very little sense to begin with, but usually can be understood as "won't crash miserably" or "will return a consistent snapshot of internal state" or some such. – millimoose Mar 14 '13 at 21:18
  • @DenisTulskiy Seeing as the cited code doesn't ever prevent that, and the documentation says no such thing, it's reasonable to assume that you can. – millimoose Mar 14 '13 at 21:21
  • @Denis Tulskiy it's allowed. Actually name was not a good example, set/get priority makes more sense, but it has the same memory consistency issue – Evgeniy Dorofeev Mar 14 '13 at 21:25

4 Answers4

4

Thread.getName() is a property than can be queried by anyone at any time. For example, a monitor utility constantly queries names of all threads. So the method has to be thread safe, otherwise, there's no clear protocol about who can safely access it and when.

Though it's always been puzzling why Thread uses a char[] to save its name, you are raising a more important question, getName() is obviously not correctly synchronized. If one thread does setName("abcd"), another thread may observe getName()->"ab\0\0".

I'll post the question to concurrency-interest list. see http://cs.oswego.edu/pipermail/concurrency-interest/2013-March/010935.html

ZhongYu
  • 19,446
  • 5
  • 33
  • 61
  • 3
    How so? Either the input to `String.valueOf()` is the original `char[]` array, or the new one, but neither actual array is edited. – sharakan Mar 14 '13 at 21:15
  • 1
    `name` could be assigned and visible to other threads, before its content is populated – ZhongYu Mar 14 '13 at 21:17
  • @zhong.j.yu Will `name` really ever be assigned *before* `toCharArray()` executes? (I suppose JIT could reorder the operations in that way but it seems odd.) – millimoose Mar 14 '13 at 21:22
  • @millimoose Yes according to java memory model. If we write that kind of code, it's a bug. But this is JDK code of a particular VM, it can assume particular execution characteristics that we cannot. – ZhongYu Mar 14 '13 at 21:25
  • @zhong.j.yu I see what you mean now. So using String here would fix the problem, because then setName would simply reset Thread.name to the an already initialized object, correct? – sharakan Mar 14 '13 at 21:58
  • No. It is not thread safe. If the thread object is shared, race conditions can occur. – xagyg Mar 14 '13 at 22:00
  • @sharakan String would be fine here since it's immutable (the fields are final). `char[N]` is more like a mutable class with N non-final `char` fields as far as Java Memory Model is concerned. – ZhongYu Mar 14 '13 at 22:00
  • @xagyg thread-safe is not a clearly defined term, usually it's up to app to define. data race does not necessarily mean thread-unsafe. – ZhongYu Mar 14 '13 at 22:01
  • @zhong.j.yu Given the small amount of information we have got, we should err on the safe side, that is, the assumption that multiple threads access the instance of the object. So, let me add some refinement, in the context of multi-threaded access, NO it is not safe. – xagyg Mar 14 '13 at 22:07
3

"API does not say anything"

If an API does not say anything, than you never can asume that a method /class is thread safe. As you can see from the source code, the acess to name is not mutual exclusive.
But for me it seems, that name has either the old value or the new one, nothing in between, so it looks safe for must uses of get/setName().

AlexWien
  • 28,470
  • 6
  • 53
  • 83
  • It's only thread-safe if the method call is only being made by a single thread. In the context of multi-threaded access (i.e. multiple threads calling get/satName()), it is not safe at all. – xagyg Mar 14 '13 at 22:09
  • @xagyg why do you think that? what do you think that would happen in the worst case? my opinion: one thred sees the old name, the other the new name, although this is theoretical, because threas names usully are asigned once. – AlexWien Mar 15 '13 at 00:37
  • Assignment of the name is not atomic, where is mutual exclusivity guaranteed? It isn't. And "although this is theoretical, because threas names usully are asigned once" - the OP is not asking the *usual* case, he/she is asking if it is safe (i.e. in all use-cases, ALWAYS). No it isn't safe. – xagyg Mar 15 '13 at 03:36
1

For starters, if one thread invokes setName while access to name isn't somehow synchronized, there's no guarantee that any other thread will ever see that new value.

Second, String.valueOf(char[]) iterates over name. That means if even one of name's characters is set during this iteration, the iterating thread might see inconsistent data - the first characters of the original char array and the last characters of the other.

In this particular case, it's not one of characters, rather it's the pointer to the beginning of the char array. Assuming that an iteration over an array computes the next cell to access by adding the current iteration index to that pointer's referred address, it will indeed also cause inconsistent data.

** EDIT **

Regarding the second non-safe scenario, after reading this question and answer, it seems much less obvious as to how actually an array copy is implemented - it is platform-dependent. This explains why String,valueOf(char[]) isn't documented as thread-safe. So, anyway, the second scenario still applies as non-thread-safe.

Community
  • 1
  • 1
yair
  • 8,945
  • 4
  • 31
  • 50
  • 1
    I just saw "it isn't thread-safe" so upvoted you. You are correct. :). You were the first person to say that. – xagyg Mar 14 '13 at 22:01
0

If the thread object is shared amongst other threads (which might be unusual, but nevertheless possible - if you programmed a solution that way), then it is NOT thread-safe. It is susceptible to race conditions, just like any other class or object.

Example:

Thread t1 = new SomeThread(); t1.start();
(new MyThread(t1, new Runnable() { public void run() {t1.setName("HELLO");} })).start();
(new MyThread(t1, new Runnable() { public void run() {t1.setName("GOODBYE");} })).start();

Two threads accessing same thread object t1. Not safe.

xagyg
  • 9,562
  • 2
  • 32
  • 29