This is not thread-safe, but it's close. The problem is that you add elements to the map after storing its reference in cached, making the incomplete map available to other callers until initialization is complete. The first step is to move the map building into a separate method.
private Map<String, String> loadAllOrderStatuses() {
final List<OrderStatuses> orderStatuses = orderStatusesDao.getAll();
final HashMap<String, String> result = new HashMap<String, String>();
for (OrderStatuses orderStatus : orderStatuses) {
result.put(orderStatus.getCode(), orderStatus.getName());
}
return result;
}
Returning the raw map to clients can also be dangerous since any caller is free to add elements to it which may or may not be seen by other threads or even corrupt the map's internal structure as viewed by other threads. If you do not control all callers of this method, you may wrap the result in an unmodifiable version at the end of the above method.
return Collections.unmodifiableMap(result);
With this, as long as you're running on Java 1.5+ where volatile is well-defined, it should be thread-safe. However, it can still be improved slightly. Volatile fields force a memory barrier on every access so it's better to write the computed value only once and minimize reads in the already-initialized code path. This can be done using a local variable as seen in the Effective Java Reloaded presentation linked by Banthar.
@Transactional(rollbackFor = Exception.class, readOnly = true)
public Map<String, String> getAllOrderStatuses() {
Map<String, String> result = cached;
if (null == result) {
synchronized (this) {
result = cached;
if (null == result) {
cached = result = loadAllOrderStatuses();
}
}
}
return result;
}
Finally, you're invoking the transaction advice on every call even though it's only needed on the first for initialization. You can avoid this by moving loadAllOrderStatuses along with @Transactional to a separate bean. There are other other workarounds workarounds if this seems too much like overkill.
This is not thread-safe, but it's close. The problem is that you add elements to the map after storing its reference in cached, making the incomplete map available to other callers until initialization is complete. The first step is to move the map building into a separate method.
private Map<String, String> loadAllOrderStatuses() {
final List<OrderStatuses> orderStatuses = orderStatusesDao.getAll();
final HashMap<String, String> result = new HashMap<String, String>();
for (OrderStatuses orderStatus : orderStatuses) {
result.put(orderStatus.getCode(), orderStatus.getName());
}
return result;
}
Returning the raw map to clients can also be dangerous since any caller is free to add elements to it which may or may not be seen by other threads or even corrupt the map's internal structure as viewed by other threads. If you do not control all callers of this method, you may wrap the result in an unmodifiable version at the end of the above method.
return Collections.unmodifiableMap(result);
With this, as long as you're running on Java 1.5+ where volatile is well-defined, it should be thread-safe. However, it can still be improved slightly. Volatile fields force a memory barrier on every access so it's better to write the computed value only once and minimize reads in the already-initialized code path. This can be done using a local variable as seen in the Effective Java Reloaded presentation linked by Banthar.
@Transactional(rollbackFor = Exception.class, readOnly = true)
public Map<String, String> getAllOrderStatuses() {
Map<String, String> result = cached;
if (null == result) {
synchronized (this) {
result = cached;
if (null == result) {
cached = result = loadAllOrderStatuses();
}
}
}
return result;
}
Finally, you're invoking the transaction advice on every call even though it's only needed on the first for initialization. You can avoid this by moving loadAllOrderStatuses along with @Transactional to a separate bean. There are other workarounds if this seems too much like overkill.
This is not thread-safe, but it's close. The problem is that you add elements to the map after storing its reference in cached, making the incomplete map available to other callers until initialization is complete. The first step is to move the map building into a separate method.
private Map<String, String> loadAllOrderStatuses() {
final List<OrderStatuses> orderStatuses = orderStatusesDao.getAll();
final HashMap<String, String> result = new HashMap<String, String>();
for (OrderStatuses orderStatus : orderStatuses) {
result.put(orderStatus.getCode(), orderStatus.getName());
}
return result;
}
Returning the raw map to clients can also be dangerous since any caller is free to add elements to it which may or may not be seen by other threads or even corrupt the map's internal structure as viewed by other threads. If you do not control all callers of this method, you may wrap the result in an unmodifiable version at the end of the above method.
return Collections.unmodifiableMap(result);
With this, as long as you're running on Java 1.5+ where volatile is well-defined, it should be thread-safe. However, it can still be improved slightly. Volatile fields force a memory barrier on every access so it's better to write the computed value only once and minimize reads in the already-initialized code path. This can be done using a local variable as seen in the Effective Java Reloaded presentation linked by Banthar.
@Transactional(rollbackFor = Exception.class, readOnly = true)
public Map<String, String> getAllOrderStatuses() {
Map<String, String> result = cached;
if (null == result) {
synchronized (this) {
result = cached;
if (null == result) {
cached = result = loadAllOrderStatuses();
}
}
}
return result;
}
Finally, you're invoking the transaction advice on every call even though it's only needed on the first for initialization. You can avoid this by moving loadAllOrderStatuses along with @Transactional to a separate bean. There are other workarounds if this seems too much like overkill.
This is not thread-safe, but it's close. The problem is that you add elements to the map after storing its reference in cached, making the incomplete map available to other callers until initialization is complete. The first step is to move the map building into a separate method.
private Map<String, String> loadAllOrderStatuses() {
final List<OrderStatuses> orderStatuses = orderStatusesDao.getAll();
final HashMap<String, String> result = new HashMap<String, String>();
for (OrderStatuses orderStatus : orderStatuses) {
result.put(orderStatus.getCode(), orderStatus.getName());
}
return result;
}
Returning the raw map to clients can also be dangerous since any caller is free to add elements to it which may or may not be seen by other threads or even corrupt the map's internal structure as viewed by other threads. If you do not control all callers of this method, you may wrap the result in an unmodifiable version at the end of the above method.
return Collections.unmodifiableMap(result);
With this, as long as you're running on Java 1.5+ where volatile is well-defined, it should be thread-safe. However, it can still be improved slightly. Volatile fields force a memory barrier on every access so it's better to write the computed value only once and minimize reads in the already-initialized code path. This can be done using a local variable as seen in the Effective Java Reloaded presentation linked by Banthar.
@Transactional(rollbackFor = Exception.class, readOnly = true)
public Map<String, String> getAllOrderStatuses() {
Map<String, String> result = cached;
if (null == result) {
synchronized (this) {
result = cached;
if (null == result) {
cached = result = loadAllOrderStatuses();
}
}
}
return result;
}
Finally, you're invoking the transaction advice on every call even though it's only needed on the first for initialization. You can avoid this by moving loadAllOrderStatuses along with @Transactional to a separate bean. There are other workarounds if this seems too much like overkill.