-
-
Notifications
You must be signed in to change notification settings - Fork 36
WIP: Multi Root Contexts #261
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
base: main
Are you sure you want to change the base?
Conversation
james-pre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jeff, thanks for the amazing work on this so far. I know this is still a work in progress but have some feedback.
One of my biggest concerns is that this PR appears to include some amount of PID namespacing— which is outside the scope of what I'm comfortable adding. I think we need to focus on just adding one feature in this PR. Mission creep is a very real problem, and one that I'd like to avoid. Moreover, there are some very broad changes to contexts' mechanics beyond just adding a mounts property to FSContext that I'm not sure are needed for the feature to work.
Also, I'd like this feature to be somewhat compatible with Linux's mount namespaces.
I apologize if this comes off as overly negative or pessimistic, however I want to make sure that we only make the changes needed for the feature: every line of code that is changed is another potential bug; every new line increases the amount of maintenance. I really hope that we can push this feature through without compromising quality.
Thanks again for putting so much work into this, I really appreciate it!
| export interface FSContext { | ||
| /** The unique ID of the context */ | ||
| readonly id: number; | ||
| readonly id: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to stay number for compatiblity with older versions and with POSIX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like formatting got messed up here. Make sure to keep tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add a bunch of extra lock files. package-lock.json is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sorry. Ignore all the extra junk (flake.nix, edits to test.js, etc) This WIP is mainly to see what you think of getContext and .createChildContext I still need to clean up the misc files, type annotations, formatting, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is for NixOS then cool, however I'm not sure we're ready for adding this tooling yet. Let's stick to adding the one feature for this PR.
| /** | ||
| * If set, sets case folding for the file system(s). | ||
| * @default null | ||
| */ | ||
| caseFold?: CaseFold; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caseFold is already part of SharedConfig, this should be removed to de-dupe.
| import { context } from './context.js'; | ||
| const mounts = context.mounts; | ||
| export { mounts }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import { context } from './context.js'; | |
| const mounts = context.mounts; | |
| export { mounts }; | |
| import { context } from './context.js'; | |
| export const mounts = context.mounts; |
| * ``` | ||
| */ | ||
| export function createIsolatedContext(options: RestrictedContextOptions = {}): BoundContext { | ||
| const rootContext = _createUnboundRootContext({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we can't just have this done via a mounts property on ContextInit?
| import { configureIsolated, fs as globalThisFs } from '@zenfs/core'; | ||
|
|
||
| // create a local fs | ||
| const {fs, path} = await configureIsolated({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally the context examples use fs for the global and ctx or context for the conext's FS. Let's stay consistent.
| async create(options: FetchOptions) { | ||
| const url = new URL(options.baseUrl); | ||
| url.pathname = normalizePath(url.pathname); | ||
| url.pathname = normalizePath(url.pathname, true); // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
Co-authored-by: James Prevett <[email protected]>
No worries. Actually quite the opposite of worries; I think you and I are still on the same page even though you probably don't feel that way at all at the moment haha. I'm just doing a bad job of presenting this PR. I also maintain open source stuff and I know the dread of getting a massive PR. I'll be breaking this up, but its easier to do that with some guidance/feedback on a tests-passing system. Most things you're pointing out, like a better fix to circular dependencies, I have already "gone down the rabbit hole" trying many alternatives. For example I HATE my I'll fix the context ID's that'll be easy (is that related to what you said about PID's?). I'll have to come back later to give a full explanation of why things like getContext seem necessary for multiple root contexts. In the meantime, I agree performance is important. What performance tests can I run to show if something will/won't be a problem? |
Yup.
I wish ZenFS had comprehensive performance tests, but alas we don't. The best thing we do have is comparing the times reported by I'm going to start working on sysfs first instead of procfs in order to avoid creating conflicts for this PR. |
Honestly, idk when I'll be able to jump back in on this so I'd say don't worry about creating conflicts. One thing though, a bug that would be my first actual PR: I think Later I'll explain why |
|
I'll probably make some of the minor changes / bug fixes (e.g. sorting out |
The design/approach/API is ready for critique, all tests pass.
Edit: now that I think about it, I should probably explain the changes. A lot of stuff had to be done to prevent circular dependencies. But I'll have to do that later.
Warning:
I just wanted to get this out so you could take a look in the meantime (might be several weeks/months before I get enough free time to check back in on this)