Bug 62768

Summary: OPCPackage#close() method is incorrectly synchronized
Product: POI Reporter: Danila Galimov <bgrh>
Component: OPCAssignee: POI Developers List <dev>
Severity: major    
Priority: P2    
Version: 4.0.0-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: multi-thread runner

Description Danila Galimov 2018-09-27 17:55:58 UTC
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 {
} finally {

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.
Comment 2 Dominik Stadler 2018-09-27 20:29:57 UTC
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.
Comment 3 Danila Galimov 2018-09-27 20:52:15 UTC
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.
Comment 4 PJ Fanning 2018-09-27 21:18:25 UTC
I'll remove the synchronized keyword