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.

Sign up for GitHub

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

Jump to bottom

Don't close a ZipEntryStorage archive file when obtaining its name #486

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Don't close a ZipEntryStorage archive file when obtaining its name #486

merged 1 commit into from
Sep 12, 2024

Conversation

HannesWell
Copy link
Contributor

What it does

Don't close a ZipEntryStorage archive file when obtaining its name to show a label.

The setup where this happens not not very simple and I therefore didn't replicated this in a clean debug environment yet, but from looking at all callers of ZipEntryStorage.getArchive(), this seems to be the only one closing it.
So this is just an anticipated fix that hopefully fixes eclipse-platform/eclipse.platform.ui#2251

Of course it could also be some code that is holding a reference to the archive from before it was passed to a ZipEntryStorage.

This basically reverts 876dd12 done for Bug 558863

How to test

The setup where this happens not not very simple and I was not yet able to replicate and test this.

Author checklist

This was referenced Sep 7, 2024
Merged
Closed
HannesWell
HannesWell commented Sep 7, 2024
View reviewed changes
...t.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/launcher/SourceElementQualifierProvider.java Outdated
Comment on lines 50 to 52
@SuppressWarnings("resource") // Don't close the archive to not break subsequent usages
String zipFileName = storage.getArchive().getName();
IPath path = IPath.fromOSString(zipFileName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the new API introduce in eclipse-platform/eclipse.platform#1545, this could avoid the call to getArchive() and would avoid suppressing the warning.
Therefore I suggest we await that.

@HannesWell
Copy link
Contributor Author

eclipse-platform/eclipse.platform#1545 is now submitted and available after the next I-build.
With that this should be ready.

@iloveeclipse or @akurtakov can you please have a look at this?

@jukzi
Copy link
Contributor

jukzi commented Sep 11, 2024

i guess org.eclipse.debug.internal.ui.sourcelookup.SourceElementWorkbenchAdapter.getLabel(Object) needs also be updated

@jukzi
Copy link
Contributor

jukzi commented Sep 11, 2024

i guess org.eclipse.debug.internal.ui.sourcelookup.SourceElementWorkbenchAdapter.getLabel(Object) needs also be updated

forget that comment. it already is.

jukzi
jukzi approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

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

lgtm. just need to wait for a build

@jukzi
Copy link
Contributor

jukzi commented Sep 11, 2024 

12:43:47  [ERROR] /home/jenkins/agent/workspace/eclipse.jdt.debug-github_PR-486/org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/launcher/SourceElementQualifierProvider.java:[51] 
12:43:47  [ERROR] 	Path zipFile = storage.getArchivePath;
12:43:47  [ERROR] 	                       ^^^^^^^^^^^^^^
12:43:47  [ERROR] getArchivePath cannot be resolved or is not a field

probably you need to increase the required bundle version

@HannesWell
Don't close a ZipEntryStorage archive file when obtaining its name
f89dcb7 
Revert "Bug 558863 - Possible Recource leak warning in
SourceElementQualifierProvider"

This reverts commit 876dd12.

Fixes eclipse-platform/eclipse.platform.ui#2251
@HannesWell
Copy link
Contributor Author

HannesWell commented Sep 11, 2024 •
edited
Loading

lgtm. just need to wait for a build

Thank you.

12:43:47  [ERROR] /home/jenkins/agent/workspace/eclipse.jdt.debug-github_PR-486/org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/launcher/SourceElementQualifierProvider.java:[51] 
12:43:47  [ERROR] 	Path zipFile = storage.getArchivePath;
12:43:47  [ERROR] 	                       ^^^^^^^^^^^^^^
12:43:47  [ERROR] getArchivePath cannot be resolved or is not a field

probably you need to increase the required bundle version

The error was on my side. Since I did the change before the bundle was actually available in the TP and just rebased this after the I-build I didn't get an error because I forgot the braces. It should be fine now.

@jukzi jukzi merged commit a509602 into eclipse-jdt:master Sep 12, 2024
10 checks passed
@jukzi
Copy link
Contributor

jukzi commented Sep 12, 2024

thanks for the fix.

@HannesWell HannesWell deleted the no-archive-close branch September 12, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukzi jukzi jukzi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IllegalStateException: zip file closed when debugging a Maven build
2 participants
@HannesWell @jukzi