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

memory storage #306

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

memory storage #306

wants to merge 2 commits into from

Conversation

elee1766
Copy link
Contributor

@elee1766 elee1766 commented Aug 15, 2024

renames existing memoryStorage -> testingMemoryStorage, as it is a special storage only used for testing

creates memoryStorage, which can only be created through calling NewMemoryStorage

memory storage is thread safe, supports lock keyed by file

@elee1766 elee1766 changed the title noot memory storage Aug 15, 2024
@mholt
Copy link
Member

mholt commented Aug 15, 2024

Thanks! Is there a reason we need two? Like, why not just use the new memoryStorage for testing too?

@elee1766
Copy link
Contributor Author

Thanks! Is there a reason we need two? Like, why not just use the new memoryStorage for testing too?

i'm not sure.

// testingMemoryStorage is an in-memory storage implementation with known contents and fixed iteration order for List.

i assume those things matter for testing, and the map based in-memory store can't do things, so my guess is we can't.

@mholt
Copy link
Member

mholt commented Aug 23, 2024

Hmm. I still wonder if we can just use the one for both. (In other words, keep the new memoryStorage type, and just use it instead of the new testingMemoryStorage type.) I haven't had a chance to explore this but could we possibly try it if anyone has a chance?

@elee1766
Copy link
Contributor Author

elee1766 commented Aug 23, 2024

i can try to look for some time to investigate. i am usually afraid of changing existing tests.

Copy link
Contributor

@ankon ankon left a comment

Choose a reason for hiding this comment

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

The "original" memory storage was added with #283, and indeed the predictable order of the its List method is used in one of the tests (specifically the one that checks that broken accounts are correctly skipped). See also #283 (comment)

That being said, I guess the "new" memory storage is intended not so much for the purpose of unit tests, but rather actual deployments: In that case I wonder whether using a tmpfs-backed file storage might be a better approach anyways, as it would give the ability to actually "look at" the stored contents?

@@ -0,0 +1,200 @@
// Copyright 2015 Matthew Holt
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs updating?

d []byte
}

// memoryStorage is a Storage implemention that exists only in memory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// memoryStorage is a Storage implemention that exists only in memory
// memoryStorage is a Storage implementation that exists only in memory

@elee1766
Copy link
Contributor Author

elee1766 commented Oct 30, 2024

@ankon

using actual tmpfs may be an issue because of portability, is why i did not opt for this. especially on windows hosts, i don't know how to create a ramdisk, and afaik there is no tmpfs-like thing that you can easily access that exists in page cache.

it is true though, using tmpfs would reduce the memory overhead on linux (in exchange for possibly higher cpu usage, i think)

if you had thoughts for anything cross-platform that might work, would be happy to consider and implement.

I think matt would have to approve new dependencies if they were required. personally i use https://github.com/spf13/afero memory backed fs for cases like these - but i didn't want to pull the dependency in.

@ankon
Copy link
Contributor

ankon commented Nov 2, 2024

Well, indeed tmpfs would be the "Linux-thing", but the point here would be to have it out of the scope of certmagic: It simply is a question of "configure the thing you're using certmagic in such that the storage points to a suitable file system" (which, in the case of "testing" and "linux" could be a tmpfs mountpoint).

I did a quick look now, and it seems that Windows doesn't come out-of-the-box with any such functionality, so when running on Windows that wouldn't work.

I guess what I'm trying to say is: Adding a in-memory storage is an interesting idea, but I wonder whether that is something certmagic needs to have, or whether that could be easily done outside of it -- for instance a library that provides a in-memory storage (using github.com/spf13/afero, for instance) could just live in a separate repository, and certmagic could point to it in the README.

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.

3 participants