PUBLIC - Liferay Portal Community Edition
  1. PUBLIC - Liferay Portal Community Edition
  2. LPS-26756

Multithreading code issues - increment to volatile int/long aren't atomic - increments could be lost

    Details

    • Similar Issues:
      Show 5 results 

      Description

      Bug:
      1) Increment of volatile field com.liferay.portal.kernel.concurrent.ThreadPoolExecutor$WorkerTask._localCompletedTaskCount in com.liferay.portal.kernel.concurrent.ThreadPoolExecutor$WorkerTask._runTask(Runnable)
      2) Increment of volatile field com.liferay.portal.search.lucene.IndexAccessorImpl._batchCount in com.liferay.portal.search.lucene.IndexAccessorImpl._write(Term, Document)
      3) Increment of volatile field com.liferay.portal.search.lucene.IndexAccessorImpl._batchCount in com.liferay.portal.search.lucene.IndexAccessorImpl.deleteDocuments(Term)

      code increments a volatile field. Increments of volatile fields aren't atomic. If more than one thread is incrementing the field at the same time, increments could be lost.

        Activity

        Hide
        Juan Fernández added a comment -

        According to Shuyang Zhou "There are reasons that we are using volatile fields over Atomic fields for certain cases.

        The ThreadPoolExecutor.java change is totally unnecessary, because the mainLock ensures mutually exclusive accessing already, we only need volatile to ensure memory visibility across threads.

        The IndexAccessorImpl.java change is also unneeded, we can tolerate loss update for batch indexing, so volatile provides better performance."

        So I'm closing this ticket.
        Anyways, thanks a lot for your contribution. Keep your suggestions comming.
        Juan Fernández

        Show
        Juan Fernández added a comment - According to Shuyang Zhou "There are reasons that we are using volatile fields over Atomic fields for certain cases. The ThreadPoolExecutor.java change is totally unnecessary, because the mainLock ensures mutually exclusive accessing already, we only need volatile to ensure memory visibility across threads. The IndexAccessorImpl.java change is also unneeded, we can tolerate loss update for batch indexing, so volatile provides better performance." So I'm closing this ticket. Anyways, thanks a lot for your contribution. Keep your suggestions comming. Juan Fernández
        Hide
        Michael Saechang added a comment -

        Closing as per Juan's comment.

        Show
        Michael Saechang added a comment - Closing as per Juan's comment.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              3 years, 10 weeks, 5 days ago

              Development

                Structure Helper Panel