Bug 62768 - OPCPackage#close() method is incorrectly synchronized
Summary: OPCPackage#close() method is incorrectly synchronized
Alias: None
Product: POI
Classification: Unclassified
Component: OPC (show other bugs)
Version: 4.0.0-FINAL
Hardware: PC All
: P2 major (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Reported: 2018-09-27 17:55 UTC by Danila Galimov
Modified: 2018-09-27 21:18 UTC (History)
0 users

multi-thread runner (2.12 KB, text/plain)
2018-09-27 17:55 UTC, Danila Galimov

Note You need to log in before you can comment on or make changes to this bug.
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