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

Add a method to allow LifecycleManager to free keys #75

Closed
wants to merge 1 commit into from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Jan 24, 2023

Currently there is no way to ever remove a key from the map, this can lead to accumulation of memory as it is strongly referencing the class.

This adds a new method so keys can be removed from the map

Fixes #74

@cstamas @gnodet can you take a look?

@cstamas cstamas requested review from mcculls, kwin and cstamas January 24, 2023 08:05
@cstamas
Copy link
Member

cstamas commented Jan 24, 2023

FYI, this is Java 7 project

@laeubi
Copy link
Contributor Author

laeubi commented Jan 24, 2023

FYI, this is Java 7 project

FYI even Java 8 is EOL for a long time, so maybe time to upgrade :-)

But I'll adjust the code then... at least try to do so.

Currently there is no way to ever remove a key from the map, this can
lead to accumulation of memory as it is strongly referencing the class.

This adds a new method so keys can be removed from the map

See eclipse#74

Signed-off-by: Christoph Läubrich <[email protected]>
@laeubi
Copy link
Contributor Author

laeubi commented Feb 6, 2023

@cstamas rebased and changed to java 7 style

@laeubi
Copy link
Contributor Author

laeubi commented Feb 6, 2023

I have no idea why the ECA check fials, the ECA tool sais it is valid and I use it all the time for eclipse contributions...

@mcculls
Copy link
Contributor

mcculls commented Feb 7, 2023

Hi @laeubi - thanks for the PR, just wondering if a simple flush(ClassLoader) would also work? ie. remove all entries for classes from the given class-loader. It's not a big deal if that removes more entries than strictly necessary, because any missing entry will get recreated if it's ever needed again in the future.

@mcculls
Copy link
Contributor

mcculls commented Feb 7, 2023

( I also re-ran the ECA check and it passed, so seems to have been a temporary glitch )

@laeubi
Copy link
Contributor Author

laeubi commented Feb 7, 2023

I just thought it might be more flexible to check for a class, but a classloader would work for sure as well.

@mcculls mcculls self-assigned this Apr 7, 2023
@laeubi
Copy link
Contributor Author

laeubi commented Nov 29, 2023

@mcculls do you maybe want to take over the PR and just change it so it best fit the needs of sisu.inject project? I think the idea is quite clear on what to archive here?

@cstamas cstamas changed the base branch from master to main May 30, 2024 09:11
@cstamas
Copy link
Member

cstamas commented May 30, 2024

Retargeted PR against new default branch main, no other change done.

@laeubi
Copy link
Contributor Author

laeubi commented May 30, 2024

@cstamas please apply / use this as you find appropriate...

* @param remove a tester that can decide if this key needs to be flushed or
* not.
*/
public void flushCacheFor( ClassTester remove )
Copy link
Member

Choose a reason for hiding this comment

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

pls add @since 0.9.0.M3

/**
* Allows testing if a class should be flushed from the cache
*/
public static interface ClassTester
Copy link
Member

Choose a reason for hiding this comment

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

pls add @since 0.9.0.M3

@cstamas cstamas added this to the 0.9.0.M3 milestone May 30, 2024
@cstamas
Copy link
Member

cstamas commented May 31, 2024

Superseded by #138

@cstamas cstamas closed this May 31, 2024
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.

LifecycleManager reatains classes forever
3 participants