Skip to content

Closing ClassfileReader must also close its underlying Resource.#600

Merged
lukehutch merged 1 commit into
classgraph:latestfrom
chrisr3:chrisr3-close-readers
Nov 20, 2021
Merged

Closing ClassfileReader must also close its underlying Resource.#600
lukehutch merged 1 commit into
classgraph:latestfrom
chrisr3:chrisr3-close-readers

Conversation

@chrisr3

@chrisr3 chrisr3 commented Nov 19, 2021

Copy link
Copy Markdown
Contributor

Upgrading from 4.8.90 to 4.8.133, I have noticed that the resources returned by scanResult.allResources are now in an "open" state, such that invoking open() on them throws an IOException with the message:

"Resource is already open -- cannot open it again without first calling close()"

I believe I have a fix for this, and am currently trying to write some test cases.

@chrisr3 chrisr3 force-pushed the chrisr3-close-readers branch 5 times, most recently from b1e65e3 to 51ffab7 Compare November 19, 2021 16:52
@chrisr3 chrisr3 force-pushed the chrisr3-close-readers branch from 51ffab7 to 2829313 Compare November 19, 2021 17:11
@lukehutch lukehutch marked this pull request as ready for review November 20, 2021 08:35
@lukehutch lukehutch merged commit d73dd73 into classgraph:latest Nov 20, 2021
@lukehutch

Copy link
Copy Markdown
Member

@chrisr3 This is the correct fix -- thank you for this pull request, and for the unit test! Released in version 4.8.134.

@chrisr3

chrisr3 commented Nov 20, 2021

Copy link
Copy Markdown
Contributor Author

@chrisr3 This is the correct fix -- thank you for this pull request, and for the unit test! Released in version 4.8.134.

Thanks @lukehutch, although I wasn't quite expecting it to be merged just yet. I think there is still an issue with ClasspathElementModule.java because closing the InputStream returned by its Resource.open() method will not yet reset its isOpen field.

@chrisr3 chrisr3 deleted the chrisr3-close-readers branch November 20, 2021 17:27
@lukehutch

Copy link
Copy Markdown
Member

@chrisr3 Good catch, please see the latest commit above, and let me know if this resolves your concern.

I'll check the rest of the Resource implementations to make sure they're not missing any more resource close actions.

@chrisr3

chrisr3 commented Nov 20, 2021

Copy link
Copy Markdown
Contributor Author

Hi @lukehutch,

Not really, because the super.close() in Resource.close() should handle closing the InputStream already. My point was more that InputStream.close() doesn't close the Resource as it would for the other Resource types.

Chris

lukehutch added a commit that referenced this pull request Nov 20, 2021
@lukehutch

Copy link
Copy Markdown
Member

@chrisr3 Yes, I just realized this and removed the last commit :-)

I understand the concern now. I have an InputStream wrapper in ClassGraph that allows a custom close action to be added. I'll wrap it in that.

lukehutch added a commit that referenced this pull request Nov 20, 2021
lukehutch added a commit that referenced this pull request Nov 20, 2021
@lukehutch

Copy link
Copy Markdown
Member

@chrisr3 OK, closing the InputStream should close the underlying Resource now.

However I noticed another problem: the ByteBuffer returned by ModuleReaderProxy#read(String path) does not close the underlying Resource either when ModuleReaderProxy#release(ByteBuffer) is called.

Last time I tried implementing a custom ByteBuffer (which is a ton of work!), I got all the way to the end of the process and then discovered that a critical method is non-public (I forget which one) -- so it is not possible to extend ByteBuffer in user code, it is only possible for the JDK team to add new ByteBuffer instances. I may need to change the ClassGraph API to return a ByteBufferWrapper instance that holds a ByteBuffer and also a reference to the underlying Resource, so that that can be closed.

@lukehutch

Copy link
Copy Markdown
Member

I extended the Resource interface to include a readCloseable() method that returns a CloseableByteBuffer that calls Resource#close() when CloseableByteBuffer#close() is called. I think this is the best that can be done, since ByteBuffer cannot be extended (the problem is that ByteBuffer has no protected constructors).

Released these fixes in version 4.8.135. Thanks!

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.

2 participants