[CI] ConcurrentModificationException in DequeRecycler.close #41683
Assignees
Labels
Comments
davidkyle
added
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
|
I labelled this core/core please reassign if you know better |
|
and Does not reproduce |
jaymode
added a commit
to jaymode/elasticsearch
that referenced
this issue
Apr 30, 2019
This change addresses some concurrency issues that can occur when closing a DequeRecycler. The first issue that is addressed is a ConcurrentModificationException that can occur when using a locked DequeRecycler that is backed by an ArrayDeque. The ArrayDeque is not thread safe and requires external synchronization. In most cases the locked DequeRecycler handles this correctly but closing the DequeRecycler is not protected by the lock, which can lead to the ConcurrentModificationException if other threads are calling the obtain method. Additionally, the DequeRecycler close method used an iterator to go over all entries in the Deque and then later cleared them. The close method now uses pollFirst in a loop to empty the Deque and still execute the destroy method. Finally, the ConcurrentDequeRecycler had an overzealous assertion in the close method that the size of the Deque is equivalent to the externally tracked size. This assertion is not always going to be true due to the nature of the implementation. There is no lock guarding both the deque and the size value, so there is always a chance that the two could be wrong depending on ongoing requests. This assertion has been removed and a comment has been added that mentions there can be some discrepancies between the actual size of the deque and the externally tracked size. These issues may have always been present, but I believe the changes in Closes elastic#41683
jaymode
added a commit
to jaymode/elasticsearch
that referenced
this issue
Apr 30, 2019
This change addresses some concurrency issues that can occur when closing a DequeRecycler. The first issue that is addressed is a ConcurrentModificationException that can occur when using a locked DequeRecycler that is backed by an ArrayDeque. The ArrayDeque is not thread safe and requires external synchronization. In most cases the locked DequeRecycler handles this correctly but closing the DequeRecycler is not protected by the lock, which can lead to the ConcurrentModificationException if other threads are calling the obtain method. Additionally, the DequeRecycler close method used an iterator to go over all entries in the Deque and then later cleared them. The close method now uses pollFirst in a loop to empty the Deque and still execute the destroy method. Finally, the ConcurrentDequeRecycler had an overzealous assertion in the close method that the size of the Deque is equivalent to the externally tracked size. This assertion is not always going to be true due to the nature of the implementation. There is no lock guarding both the deque and the size value, so there is always a chance that the two could be wrong depending on ongoing requests. This assertion has been removed and a comment has been added that mentions there can be some discrepancies between the actual size of the deque and the externally tracked size. These issues may have always been present, but I believe the changes in Closes elastic#41683
jaymode
added a commit
to jaymode/elasticsearch
that referenced
this issue
May 8, 2019
The changes in elastic#39317 brought to light some concurrency issues in the close method of Recyclers as we do not wait for threads running in the threadpool to be finished prior to the closing of the PageCacheRecycler and the Recyclers that are used internally. elastic#41695 was opened to address the concurrent close issues but upon review, the closing of these classes is not really needed as the instances should be become available for garbage collection once there is no longer a reference to the closed node. Closes elastic#41683
|
Different test similar failure https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+intake/3496/console |
jaymode
added a commit
that referenced
this issue
May 10, 2019
The changes in #39317 brought to light some concurrency issues in the close method of Recyclers as we do not wait for threads running in the threadpool to be finished prior to the closing of the PageCacheRecycler and the Recyclers that are used internally. #41695 was opened to address the concurrent close issues but upon review, the closing of these classes is not really needed as the instances should be become available for garbage collection once there is no longer a reference to the closed node. Closes #41683
jaymode
added a commit
that referenced
this issue
May 10, 2019
The changes in #39317 brought to light some concurrency issues in the close method of Recyclers as we do not wait for threads running in the threadpool to be finished prior to the closing of the PageCacheRecycler and the Recyclers that are used internally. #41695 was opened to address the concurrent close issues but upon review, the closing of these classes is not really needed as the instances should be become available for garbage collection once there is no longer a reference to the closed node. Closes #41683
gurkankaymak
pushed a commit
to gurkankaymak/elasticsearch
that referenced
this issue
May 27, 2019
The changes in elastic#39317 brought to light some concurrency issues in the close method of Recyclers as we do not wait for threads running in the threadpool to be finished prior to the closing of the PageCacheRecycler and the Recyclers that are used internally. elastic#41695 was opened to address the concurrent close issues but upon review, the closing of these classes is not really needed as the instances should be become available for garbage collection once there is no longer a reference to the closed node. Closes elastic#41683
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The error happened in the
afterClassmethod inIPHostnameVerificationTests, I doubt it is related to the testhttps://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-unix-compatibility/os=ubuntu-18.04&&immutable/364/console
Does not reproduce and not a lot to work with sorry
Note the repo command lists the test
IPHostnameVerificationTests.null, null is not a test this must be a result of the failure occurring in the after class methodThe text was updated successfully, but these errors were encountered: