[hibernate-issues] [Hibernate-JIRA] Commented: (HSEARCH-211) SharedReaderProvider: should own lock when testing for index state

[Sanne Grinovero (JIRA)]

    [ http://opensource.atlassian.com/projects/hibernate/browse/HSEARCH-211?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_30673 ] 

Sanne Grinovero commented on HSEARCH-211:
-----------------------------------------

Hello, please I could use some help about understanding the code of SharedReaderProvider,
because my most impulsive solution would be to rewrite most of it which is obviously a bad idea.

"directoryProviderLock.lock(); //needed for same problem as the double-checked locking"
I don't understand the comment, to correctly save you from the double-checked locking issue
I think you shouldn't release the lock before assigning a new value to the reader, otherwise
it would be possible to initialize the values multiple times; what's the double-checking problem?

If I keep the lock longer it will be held even around the replaceActiveReader(...) and because of the issue
we would need to hold the lock during both branches of the "if (reader ==null)"
Also because of this issue we could be trying to reopen an already closed reader if the lock is not held during the whole
else branch, so it turns out we need to hold the lock during the whole iteration of the for cycle, without releasing it until
next iteration.
This would make the lock acquisition during replaceActiveReader useless, removing the need
for the locks on semaphores as we are holding already wider-scope locks.

Having this longer locks won't necessarily hurt performance, as they would also stop different threads from doing the same
expensive operations twice (or more) such as testing index state and opening new readers.
There's a note about scalability, IMHO having a second thread waiting for the first to finish the same job instead of having to
do the same stuff is usually better.

if the semaphoreLocks are needed, then:
semaphoreIndexReaderLock is used to protect a HashMap (which could use a ConcurrentHashMap) and at the same
time to protect readerSemaphores, even different semaphores are not permitted to be updated concurrently;
it look like too restrictive, a per-dir lock could be used or the semaphores could use the real semaphore class
instead of int, or the AtomicInteger.
at last, the locks are currently not guaranteed to be visible to all threads: same care as already discussed for HSEARCH-189
is needed.

Any comments about how I should proceed? IMHO I'll put the broader locks, I think they are needed.

> SharedReaderProvider: should own lock when testing for index state
> ------------------------------------------------------------------
>
>                 Key: HSEARCH-211
>                 URL: http://opensource.atlassian.com/projects/hibernate/browse/HSEARCH-211
>             Project: Hibernate Search
>          Issue Type: Bug
>          Components: directory provider
>            Reporter: Sanne Grinovero
>            Assignee: Sanne Grinovero
>             Fix For: 3.1.0.Beta1
>
>
> Developing a new IndexReader I made a stress-test to compare performance to base readers, but got this when measuring SharedReaderProvider:
> Exception in thread "pool-2-thread-13" org.apache.lucene.store.AlreadyClosedException: this IndexReader is closed
> 	at org.apache.lucene.index.IndexReader.ensureOpen(IndexReader.java:163)
> 	at org.apache.lucene.index.DirectoryIndexReader.isCurrent(DirectoryIndexReader.java:149)
> 	at org.hibernate.search.reader.SharedReaderProvider.openReader(SharedReaderProvider.java:83)
> 	at org.hibernate.search.query.FullTextQueryImpl.buildSearcher(FullTextQueryImpl.java:486)
> 	at org.hibernate.search.query.FullTextQueryImpl.getResultSize(FullTextQueryImpl.java:528)
> 	at org.hibernate.search.test.reader.performance.SearchActivity.doAction(SearchActivity.java:24)
> 	at org.hibernate.search.test.reader.performance.AbstractActivity.run(AbstractActivity.java:54)
> 	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:650)
> 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:675)
> 	at java.lang.Thread.run(Thread.java:595)

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://opensource.atlassian.com/projects/hibernate/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira