Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(allocated/buffer): release allocated direct memory on close #8632

Closed
wants to merge 1 commit into from

Conversation

romansmirnov
Copy link
Member

@romansmirnov romansmirnov commented Jan 21, 2022

Description

Release allocated direct memory when closing the dispatcher.

Related issues

closes #8509

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @romansmirnov good catch ! 🎖️

I know that we removed the cleaner stuff at some point, because it was a bit hard to maintain and should actually be done by the jdk (on gc). At that time agrona didn't support that functionallity afaik. I think agrona uses the same logic we used, which depends on unsafe (another reason why we removed it). The jdk has currently no option to free memory in an easier way. @oleschoenburg do you think this might be an issue for the jdk 17 migration?

If ole has sees no issues than I think it is fine :)

@lenaschoenburg
Copy link
Member

lenaschoenburg commented Jan 21, 2022

As we are already running on JDK 17 it's probably fine. I don't expect any issue due to raising the language level.
I also like that we can just rely on agrona to maintain this 👍

@romansmirnov
Copy link
Member Author

Thanks for your input. It seems that this change causes test failures. I am looking into it.

@romansmirnov
Copy link
Member Author

Hm... now what could happen is the CommandApiRequestHandler tries to write to the Dispatcher which got freed. Meaning, the write buffer might still be in use by other components while the direct memory is freed. In that case, the request handler writes a command to an illegal address which results in exiting the JVM.

The direct byte buffer would need to be freed only when it is sure that nobody tries to write to it.

@Zelldon & @oleschoenburg, do you see any other option?

@romansmirnov romansmirnov marked this pull request as draft January 21, 2022 14:44
@romansmirnov
Copy link
Member Author

Closing this PR for now without merging it, because it causes some subsequent issues. We will come back to the issue again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple OOM encountered on benchmark cluster
3 participants