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

Resource loaders and null returns #366

Closed
dmlloyd opened this issue Oct 29, 2024 · 2 comments
Closed

Resource loaders and null returns #366

dmlloyd opened this issue Oct 29, 2024 · 2 comments

Comments

@dmlloyd
Copy link
Collaborator

dmlloyd commented Oct 29, 2024

There's an ambiguity with ResourceLoader. When a resource is not found, findResource may return null. However, in some cases we're instead returning a Resource which may later fail with e.g. NoSuchFileException.

Specifically:

  • In the JAR resource loader, we return a valid Resource if the relative resource path is a file in the JAR or is a directory that physically exists in the JAR. For directories that are not existent as entries in the JAR, null is returned, even though we may want to be able to iterate those "virtual" directories.
  • In the Path resource loader, we return a Resource for any valid path even if the corresponding filesystem file or directory does not exist. Errors are checked when the resource is opened.
  • In the URL resource loader, we return a Resource for any valid path. The resource only accesses the remote URL when content is requested. Directories are not supported by URLResourceLoader.

To fix this, at a minimum, the documentation should be updated to indicate that a Resource may be returned even if there is no resource. We might consider adding an exists() method to Resource.

Unfortunately I don't think it's realistic to alter the contract of findResource to never return null, because the return value is already being checked for null by all existing consumers. This altering findResource to always return a Resource even if we know the path is non-existent would probably not be a useful change to make.

@dmlloyd
Copy link
Collaborator Author

dmlloyd commented Oct 29, 2024

It might be a good idea to alter the filesystem resource loader so that if a file does not exist, null is returned. Obviously this is not a race-free check. It could be made race-free by opening the file early and automatically cleaning it up if the Resource is discarded; however, I think that it is likely that a Resource will be immediately used rather than cached, in which case the slight raciness could be OK. It would probably be better than having to hang on to e.g. WeakReference or Cleaner instances at any rate.

@dmlloyd
Copy link
Collaborator Author

dmlloyd commented Oct 31, 2024

Fixed with a combination of #367 and #369.

@dmlloyd dmlloyd closed this as completed Oct 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

No branches or pull requests

1 participant