Refactor EncryptedDiskStore to return io.Reader instead of io.ReadCloser #4329
Conversation
On Windows if Load() is called, the returned reader isn't closed and then Save() is called, Save() would fail. By returning just a io.Reader this possible failure scenario is prevented.
AndersonQ
added
Team:Elastic-Agent
|
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
| @@ -97,7 +97,7 @@ func (d *DiskStore) Save(in io.Reader) error { | |||
| } | |||
|
|
|||
| // Load return a io.ReadCloser for the target file. | |||
| func (d *DiskStore) Load() (io.ReadCloser, error) { | |||
| func (d *DiskStore) Load() (io.Reader, error) { | |||
There was a problem hiding this comment.
you sure this is allright?
There was a problem hiding this comment.
I can see that using os.ReadFile instead of opening the file with os.O_RDONLY removes the need for Close() on the io.Reader we return from Load(), however looking at the code it seems that in most places the Reader (or ReadCloser) is passed to config.NewConfigFrom() which calls io.ReadAll() before unmarshalling.
With this modification we are doubling the buffers with the diskstore content (one in the new ByteReader + the one in config.NewConfigFrom, I am worried that we are churning more memory as the EncryptedDiskStores are created in many places throughout the code.
Maybe we should have a small benchmark of EncryptedDiskStore.Load() + config.NewConfigFrom to have a better idea of the memory impact
What does this PR do?
Refactors EncryptedDiskStore to return io.Reader instead of io.ReadCloser
Why is it important?
Checklist
[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have added tests that prove my fix is effective or that my feature works~~[ ] I have added an entry in./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testRelated issues
error parsing version ""if agent restarts before the upgrade starts #3912Questions to ask yourself