Update AttributeMap.java #4404
Update AttributeMap.java #4404
Conversation
| attributes.values().forEach(this::shutdownIfExecutorService); | ||
| } | ||
| public void close() { | ||
| attributes.values().stream() |
There was a problem hiding this comment.
I would comment why we need to explicitely exclude ExecutorService
| attributes.values().stream() | |
| // Starting from JDK 19, ExecutorService implements AutoCloseable and will await termination on close | |
| // When ExecutorService is provided by the user, it can result in thread deadlocks if the ExecutorService | |
| // is not closed from another thread. ExecutorService are shutdown without waiting for termination | |
| attributes.values().stream() |
| .filter(value -> !(value instanceof ExecutorService)) | ||
| .forEach(v -> IoUtils.closeIfCloseable(v, null)); | ||
| attributes.values().stream() | ||
| .filter(value -> value instanceof ExecutorService) |
There was a problem hiding this comment.
Well, do a filter and later check again in shutdownIfExecutorService method is not a good deal. I would remove this filter or change the method.
I prefer something like this, instead of iterate twice:
attributes.values().forEach(attribute -> attribute instanceof ExecutorService ?
((ExecutorService)attribute.shutdown()) : IoUtils.closeIfCloseable(attribute, null));
|
Hi, thanks for the PR. This change makes sense to me; I like @tiny-george's suggestion, can you please implement that? |
@dagnir Doesn't look like this PR has been updated in a while, so I created another one that addresses the requested changes and adds some tests: #4649. Mind taking a look? |
|
Closing this as #4649 has been merged. Thank you for the PR @RANJITHROSAN17. |
Motivation and Context
This change is motivated by the need to prevent a deadlock issue that occurs in JDK 19+ when attempting to close an
ExecutorServiceincluded in theAttributeMap'sclosemethod. The current implementation can lead to a deadlock when closing resources. To resolve this issue, the code has been modified to explicitly excludeExecutorServiceinstances from theAutoCloseableloop inAttributeMap::close.Modifications
AttributeMap::closemethod to separateExecutorServiceinstances from otherAutoCloseableresources, ensuring that they are not closed together. This change prevents potential deadlocks when closing resources.shutdownIfExecutorServicemethod to explicitly shut downExecutorServiceinstances if needed.Testing
The changes have been tested thoroughly in a development environment. Testing included:
ExecutorServicein the same thread.Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License