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

Manage Local Storage binding file not found #2374

Closed
federicocodo opened this issue Dec 19, 2022 · 7 comments · Fixed by #2449
Closed

Manage Local Storage binding file not found #2374

federicocodo opened this issue Dec 19, 2022 · 7 comments · Fixed by #2449
Assignees
Labels
Milestone

Comments

@federicocodo
Copy link

federicocodo commented Dec 19, 2022

I started to use the Local Storage binding for my service application.
During my tests I notice that if I try to retrive a file that doesn't exist yet, I expect to receive a NotFound exception or a BindingResponse with the Data field null, instead I received an Internal exception just like in the image.
Would it be possible to fix to receive one of the two proposals?
Also is it possible to add in the yaml component a metadata field, time to live, for the files?

MicrosoftTeams-image

Another bug I found during my tests is that if I get the data inside a file and then delete this, I receive an exception in which is described how my file cannot be delete because it is used by another process:

image

Maybe is it cause because the file isn't close after getting the data information?

@federicocodo
Copy link
Author

federicocodo commented Jan 16, 2023

Any update to this problem?

@berndverst
Copy link
Member

@ItalyPaleAle any quick thoughts? Maybe your PR already addressed this by chance.

@berndverst berndverst added the P1 label Jan 18, 2023
@berndverst berndverst added this to the v1.10 milestone Jan 18, 2023
ItalyPaleAle added a commit to ItalyPaleAle/dapr-components-contrib that referenced this issue Jan 18, 2023
- Close file descriptor after reading a file
- Report a more specific error if the file doesn't exist

Signed-off-by: ItalyPaleAle <[email protected]>
@ItalyPaleAle
Copy link
Contributor

@federicocodo Thanks for the report.

  1. On the first issue, we can't change the returned status code sadly, nor we can return an empty data (in that case, it wouldn't be possible to distinguish whether the file doesn't exist or it exists but it's empty). However, I've changed the code so the error of the exception contains file not found: <path> and you can look at that.
  2. On the second issue, looks like a file descriptor was not closed. Fixed

Fixes in #2449 and will be shipped with Dapr 1.10

@federicocodo
Copy link
Author

@ItalyPaleAle Thanks a lot for the fixes, however what about the option to add in the yaml component or in the metadata field of the request class a time to live for files?

@berndverst
Copy link
Member

@ItalyPaleAle Thanks a lot for the fixes, however what about the option to add in the yaml component or in the metadata field of the request class a time to live for files?

I'm not in favor of that feature as a maintainer right now. That doesn't really fit the paradigm of this component. I want to avoid to introduce a separate thread for watching expiration times of files and then deleting them.

File a separate feature request for that please and we can consider it in the future.

@ItalyPaleAle
Copy link
Contributor

I agree with @berndverst . However we should probably consider offering a state store for accessing local files. The TTL could be a feature of that.

@berndverst
Copy link
Member

Let's not brainstorm that here @ItalyPaleAle.

Right now I'm not convinced that files + TTL should be combined in any form. 😅 but if there is enough demand, sure.

dapr-bot pushed a commit that referenced this issue Jan 18, 2023
* bindings.localstorage: enforcements on rootPath

Also includes ability to disallow certain root paths such as /var/run/secrets

Fixes #2444

Signed-off-by: ItalyPaleAle <[email protected]>

* Fixed tests on Windows

Signed-off-by: ItalyPaleAle <[email protected]>

* Fixed tests on Windows pt 2

Signed-off-by: ItalyPaleAle <[email protected]>

* 💄

Signed-off-by: ItalyPaleAle <[email protected]>

* Fixes #2374

- Close file descriptor after reading a file
- Report a more specific error if the file doesn't exist

Signed-off-by: ItalyPaleAle <[email protected]>

* Updated cert tests

Signed-off-by: ItalyPaleAle <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants