Các cách để cải thiện mã đó chỉ sử dụng JDK (6) các lớp được cung cấp? (Đồng thời, an toàn thread)

Question

(sơ bộ lưu ý: có lẽ đây là phù hợp hơn cho codereview?)

EDITAnswer to self; Tôi tin rằng câu trả lời này bao gồm tất cả các nhu cầu/vấn đề của tôi, nhưng tất nhiên, nhận xét được hoan nghênh. Câu hỏi gốc bên dưới để tham khảo.

Xin chào,

Quan tâm ở đây là phương pháp .getSources(). Phương thức này có nghĩa là trả về một danh sách các nguồn tin nhắn cho một Locale nhất định.

Hai cấu trúc dữ liệu trung tâm cho phương pháp này là sources và failedLookups, xem mã cho nhận xét.

thực hiện đặc biệt này của .getSources() chỉ bao giờ có thể trở lại hoặc là một danh sách trống hoặc một danh sách phần tử duy nhất, tùy thuộc vào tryAndLookup() phương pháp mà nguyên mẫu là:

protected abstract MessageSource tryAndLookup(final Locale locale) 
    throws IOException; 

Ngay bây giờ, logic của mã này là như sau:

• nếu nguồn tin nhắn cho ngôn ngữ đó đã được tra cứu thành công, nó sẽ được trả về;
• từ thời điểm này trở đi, không có tra cứu nào được thực hiện; tuy nhiên không biết liệu điều này có nghĩa là một lần tra cứu trước đó đã được thực hiện: kiểm tra tập hợp tra cứu không thành công, nếu ngôn ngữ tra cứu nằm trong đó, đó là lỗi đã biết, trả về danh sách trống;
• bây giờ, tình huống đã biết là tra cứu ngôn ngữ này thực sự chưa bao giờ được thực hiện: thực hiện nó; tùy thuộc vào kết quả của phương pháp tryAndLookup, ghi lại thành công hoặc thất bại.

Bây giờ, tại sao tôi đi đến độ dài như vậy: Tôi không kiểm soát được tryAndLookup(); có thể mất một lượng thời gian để thực thi trước khi trả về một nguồn hợp lệ hoặc không thành công. Kết quả là, tôi không muốn sử dụng khóa thô hoặc khối synchronized.

/** 
* Set of locales known to have failed lookup. 
* 
* <p>When a locale is in this set, it will not attempt to be reloaded.</p> 
*/ 
private final Set<Locale> lookupFailures 
    = new CopyOnWriteArraySet<Locale>(); 

/** 
* Set of message sources successfully looked up 
* 
* <p>When a source is in there, it is there permanently for now.</p> 
*/ 
private final ConcurrentMap<Locale, MessageSource> sources 
    = new ConcurrentHashMap<Locale, MessageSource>(); 

@Override 
protected final List<MessageSource> getSources(final Locale locale) 
{ 
    MessageSource source = sources.get(locale); 

    /* 
    * If found, return it 
    */ 
    if (source != null) 
     return Arrays.asList(source); 

    /* 
    * If it is a registered failure, return the empty list 
    */ 
    if (lookupFailures.contains(locale)) 
     return Collections.emptyList(); 

    /* 
    * OK, try and look it up. On success, register it in the sources map. 
    * On failure, record the failure an return the empty list. 
    */ 
    try { 
     source = tryAndLookup(locale); 
     sources.putIfAbsent(locale, source); 
     // EDIT: fix for bug pinpointed by JBNizet 
     // was: 
     // return Arrays.asList(source); 
     // now is: 
     return Arrays.asList(sources.get(locale)); 
    } catch (IOException ignored) { 
     lookupFailures.add(locale); 
     return Collections.emptyList(); 
    } 
} 

Câu hỏi của tôi ở đây có ba điểm:

• Tôi cố tình hạn chế bản thân mình đến các lớp học chỉ dành cho JDK; Tôi đã chọn ConcurrentHashMap làm triển khai ConcurrentMap và CopyOnWriteArraySet làm triển khai an toàn thread Set; từ javadoc, đây là những điều tốt nhất tôi có thể tìm thấy. Nhưng tôi có bị lừa dối ở đâu đó không?
• I nghĩ mã này là cuối cùng là chủ đề an toàn; một số trường hợp góc có thể dẫn đến tra cứu được thực hiện nhiều hơn một lần ví dụ, nhưng sau đó đây là lý do tại sao tôi làm .putIfAbsent(); ngay bây giờ tôi đã luôn sử dụng và tin cậy, số LoadingCache của Guava cho mục đích lưu vào bộ nhớ cache và đây là lần đầu tiên tôi bước ra khỏi lãnh thổ này; mã này có thực sự an toàn không?
• mã này có lỗi nghiêm trọng: nhiều hơn một luồng có thể đang thực thi tryAndLookup() cùng một lúc ... Giải pháp nào tồn tại để phương pháp này được thực thi chỉ một lần cho mỗi lần tra cứu?

Answer 1

Trả lời tự ...

Thuật toán đã được sửa lại hoàn toàn. Nó được dựa trên của JDK FutureTask, vì nó có hai đặc tính rất đẹp:

• phương pháp .run() của nó là không đồng bộ;
• nó là trạng thái, thành công cũng như về thất bại - có nghĩa là, nó sẽ trả về kết quả đã tính hoặc ngoại lệ được ném (được bao bọc thành ExecutionException) trên .get().

này có khá một ngụ ý về cấu trúc dữ liệu được sử dụng:

• các lookupFailures bộ tồn tại trước đó, trong đó ghi nhận những thất bại, đã biến mất;
• bản đồ hiện tại, là ConcurrentMap, hiện được thay thế bằng đồng bằng Map, được bảo vệ bởi ReentrantLock; giá trị của nó bây giờ là FutureTask<MessageSource> thay vì MessageSource.

này cũng có khá một ngụ ý về thuật toán, mà là đơn giản hơn nhiều:

• khóa bản đồ;
• tra cứu nhiệm vụ cho ngôn ngữ; nếu trước đó chưa tồn tại, hãy tạo nó và .run() nó;
• mở khóa bản đồ;
• .get() kết quả: trả về một danh sách thành phần đơn lẻ thành công, một danh sách trống về lỗi.

Toàn mã, với ý kiến:

@ThreadSafe 
public abstract class CachedI18NMessageBundle 
    extends I18NMessageBundle 
{ 
    /** 
    * Map pairing locales with {@link FutureTask} instances returning message 
    * sources 
    * 
    * <p>There will only ever be one task associated with one locale; we 
    * therefore choose to make it a normal map, guarded by a {@link 
    * ReentrantLock}.</p> 
    * 
    * <p>The tasks' {@link FutureTask#run()} method will be executed the first 
    * time this object is initialized.</p> 
    */ 
    @GuardedBy("lock") 
    private final Map<Locale, FutureTask<MessageSource>> lookups 
     = new HashMap<Locale, FutureTask<MessageSource>>(); 

    /** 
    * Lock used to guarantee exclusive access to the {@link #lookups} map 
    */ 
    private final Lock lock = new ReentrantLock(); 

    @Override 
    protected final List<MessageSource> getSources(final Locale locale) 
    { 
     FutureTask<MessageSource> task; 

     /* 
     * Grab an exclusive lock to the lookups map. The lock is held only for 
     * the time necessary to grab the FutureTask or create it (and run it) 
     * if it didn't exist previously. 
     * 
     * We can do this, since FutureTask's .run() is asynchronous. 
     */ 
     lock.lock(); 
     try { 
      /* 
      * Try and see whether there is already a FutureTask associated with 
      * this locale. 
      */ 
      task = lookups.get(locale); 
      if (task == null) { 
       /* 
       * If not, create it and run it. 
       */ 
       task = lookupTask(locale); 
       lookups.put(locale, task); 
       task.run(); 
      } 
     } finally { 
      lock.unlock(); 
     } 

     /* 
     * Try and get the result for this locale; on any failure event (either 
     * an IOException thrown by tryAndLookup() or a thread interrupt), 
     * return an empty list. 
     */ 
     try { 
      return Arrays.asList(task.get()); 
     } catch (ExecutionException ignored) { 
      return Collections.emptyList(); 
     } catch (InterruptedException ignored) { 
      return Collections.emptyList(); 
     } 
    } 

    protected abstract MessageSource tryAndLookup(final Locale locale) 
     throws IOException; 

    @Override 
    public final Builder modify() 
    { 
     throw new IllegalStateException("cached bundles cannot be modified"); 
    } 

    /** 
    * Wraps an invocation of {@link #tryAndLookup(Locale)} into a {@link 
    * FutureTask} 
    * 
    * @param locale the locale to pass as an argument to {@link 
    * #tryAndLookup(Locale)} 
    * @return a {@link FutureTask} 
    */ 
    private FutureTask<MessageSource> lookupTask(final Locale locale) 
    { 
     final Callable<MessageSource> callable = new Callable<MessageSource>() 
     { 
      @Override 
      public MessageSource call() 
       throws IOException 
      { 
       return tryAndLookup(locale); 
      } 
     }; 

     return new FutureTask<MessageSource>(callable); 
    } 
} 

tôi thậm chí có thể kiểm tra "nhiệm vụ duy nhất tạo ra", về thành công tra cứu cũng như thất bại, vì tryAndLookup() là trừu tượng, do đó "spyable" sử dụng Mockito. Nguồn bài kiểm tra đầy đủ here (phương pháp onlyOneTaskIsCreatedPer*LocaleLookup()).

Answer 2

Là một thay thế cho CopyOnWriteArraySet, bạn có thể sử dụng một ConcurrentHashMap với các giá trị vô nghĩa (ví dụ như luôn luôn sử dụng Boolean.TRUE như giá trị - bạn chỉ quan tâm đến các phím), hoặc nếu không bạn có thể sử dụng một ConcurrentSkipListSet mà bản chất là một đồng thời TreeSet sử dụng skiplist thay vì cây nhị phân cân bằng.

Giả sử rằng tryAndLookup là nhanh và không có bất kỳ tác dụng phụ nào, nó không thực sự quan trọng nếu bạn thỉnh thoảng thực thi nó nhiều lần, như vậy mã "cuối cùng chỉ an toàn" của bạn là an toàn chỉ trong ý nghĩa rằng nó sẽ không tạo ra bất kỳ hành vi bất thường nào. (Nếu chậm thì có thể sử dụng khóa hiệu quả hơn để đảm bảo rằng bạn thực thi nó nhiều lần nhất có thể, nhưng trong trường hợp này mã của bạn vẫn không có hành vi bất thường. Nếu nó có tác dụng phụ thì bạn có thể có một cuộc chạy đua dữ liệu nếu bạn thực hiện nó hai lần vào miền địa phương tương tự.)

Sửa vì tryAndLookup có thể có tác dụng phụ, bạn có thể đồng bộ hóa trên locale, hoặc nếu không bạn có thể thay đổi phương pháp của bạn như sau

private final MessageSource nullSource = new MessageSource(); // Used in place of null in the ConcurrentHashMap, which does not accept null keys or values 

protected final List<MessageSource> getSources(final Locale locale) { 
... 

    try { 
     if(sources.putIfAbsent(locale, nullSource) == null) { 
      source = tryAndLookup(locale); 
      sources.replace(locale, nullSource, source); 
      return Arrays.asList(sources.get(locale)); 
     } else { 
      // Another thread is calling tryAndLookup, so wait for it to finish 
      while(sources.get(locale) == nullSource && !lookupFailures.contains(locale)) 
       Thread.sleep(500); 
      } 
      if(sources.get(locale) != nullSource) { 
       return Arrays.asList(sources.get(locale)); 
      } else { 
       return Collections.emptyList(); 
      } 
     } 
    } catch (IOException ignored) { 
     lookupFailures.add(locale); 
     return Collections.emptyList(); 
    } 
} 

Answer 3

Bạn có thể chạy hai bước đầu tiên trong khối được đồng bộ hóa bằng cách sử dụng các loại dữ liệu đơn giản hơn. Nếu trong các bước bạn tìm thấy, bạn cần thực hiện tryAndLookup(), lưu trữ Future cho kết quả đang chờ xử lý trong danh sách tra cứu đang chờ xử lý riêng trước khi rời khỏi khối được đồng bộ hóa.

Sau đó, bên ngoài khối được đồng bộ hóa, thực hiện tra cứu thực tế. Các chủ đề tìm thấy chúng cần kết quả tương tự sẽ tìm thấy Tương lai và có thể get() kết quả của nó bên ngoài khối được đồng bộ hóa.