| Summary: | OPCPackage#close() method is incorrectly synchronized | ||
|---|---|---|---|
| Product: | POI | Reporter: | Danila Galimov <bgrh> |
| Component: | OPC | Assignee: | POI Developers List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | major | ||
| Priority: | P2 | ||
| Version: | 4.0.0-FINAL | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | All | ||
| Attachments: | multi-thread runner | ||
I am wondering why we even use synchronization here at all. Our thread-safety guarantee does not allow to use one workbook/doc/item in more than one thread at a time anyway. So this synchronization is not really necessary as we ask our users to perform synchronization at an upper level anyway, if they want to access documents across different threads... Having all of POI fully thread-safe would require many many points of synchronization and would cause a performance penalty for many current power-users. Yes, I agree, maybe the synchronization should be removed at all, but the classes should be annotated that they are designed to be used in single-thread environment only. I just opened the file and found dummy synchronization which won't work anyway, that's why i reported this. I'll remove the synchronized keyword |
Created attachment 36168 [details] multi-thread runner There is a code in OPCPackage#close() method, which creates and uses a lock: ReentrantReadWriteLock l = new ReentrantReadWriteLock(); try { l.writeLock().lock(); ... } finally { l.writeLock().unlock(); } However, it is completely useless since the *new* lock object is always created for any method execution, thus voiding this lock. Suggested change - either move l to a class field and initialize it in constructor, or simply use synchronized section as read/write lock is not really needed here. I attached the sample which runs close() method from multiple thread - as you can see, they are not synchronized with each other.