Skip to content

Ensure pointers indirected from Memory and pointing into Memory should retain originating object#1326

Merged
matthiasblaesing merged 2 commits intojava-native-access:masterfrom
matthiasblaesing:share_internal_memory
Mar 14, 2021
Merged

Ensure pointers indirected from Memory and pointing into Memory should retain originating object#1326
matthiasblaesing merged 2 commits intojava-native-access:masterfrom
matthiasblaesing:share_internal_memory

Conversation

@matthiasblaesing
Copy link
Copy Markdown
Member

The use case is this:

  • native supplies the required size of a memory region
  • a Memory object is created with the right memory size
  • native fills the memory with a structure and additional objects,
    that are held in that region, but are only referenced from the
    structure (one such example would be a Structure.ByReference)

As long as the resulting structure stays strongly referenced from Java,
all is good. When the toplevel structure goes ouf of scope, the memory
backing the strucure is not strongly referenced anymore and will be
freeed by GC.

This change fixes the issue by ensuring, that the pointers used in
substructures are SharedMemory objects, holding a strong references to
the original Memory object.

@matthiasblaesing
Copy link
Copy Markdown
Member Author

@dbwiddis could you please have a look and give it a try/get it tested? This replaces #1325, at least it should work and fix more than the single observed case.

Comment thread src/com/sun/jna/Memory.java Outdated
@dblock
Copy link
Copy Markdown
Member

dblock commented Mar 13, 2021

This is A+, very nice work @matthiasblaesing. I was following the discussion out of curiosity, and thinking there has to be a better way to do this, and this is it.

Copy link
Copy Markdown
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

I really like this solution. A few suggestions inline.

Comment thread src/com/sun/jna/Memory.java
Comment thread src/com/sun/jna/Memory.java Outdated
Comment thread src/com/sun/jna/Memory.java Outdated
@dbwiddis
Copy link
Copy Markdown
Contributor

dbwiddis commented Mar 14, 2021

Testing: Set up an Azure VM (Windows Server 2012 R2) and created a test method exercising the current (Advapi32Util.getTokenAccount) code in a loop, running as admin and collecting usernames/domains of the user of every running process. Ran with very small heap (2500K) to maintain pressure on the GC. OpenJDK 15.0.2.

Comment thread test/com/sun/jna/MemoryTest.java
@matthiasblaesing
Copy link
Copy Markdown
Member Author

@dbwiddis @dblock thank you both for review. I pushed an update, that should cover the points raised (I plan to partially squash the commits prior to merging, but I want to preserve review history until done)

@dbwiddis
Copy link
Copy Markdown
Contributor

LGTM!

@dblock
Copy link
Copy Markdown
Member

dblock commented Mar 14, 2021

Looks great, merge at will.

…n originating object

The use case is this:

- native supplies the required size of a memory region
- a Memory object is created with the right memory size
- native fills the memory with a structure _and_ additional objects,
  that are held in that region, but are only referenced from the
  structure (one such example would be a Structure.ByReference)

As long as the resulting structure stays strongly referenced from Java,
all is good. When the toplevel structure goes ouf of scope, the memory
backing the strucure is not strongly referenced anymore and will be
freeed by GC.

This change fixes the issue by ensuring, that the pointers used in
substructures are SharedMemory objects, holding a strong references to
the original Memory object.
@matthiasblaesing matthiasblaesing merged commit 0de51b3 into java-native-access:master Mar 14, 2021
@matthiasblaesing matthiasblaesing deleted the share_internal_memory branch August 10, 2021 23:14
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.

3 participants