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

In-process server #425

Merged
merged 7 commits into from
Aug 12, 2024
Merged

In-process server #425

merged 7 commits into from
Aug 12, 2024

Conversation

Ralith
Copy link
Owner

@Ralith Ralith commented Jul 29, 2024

Towards #405.

Rough in some places and not actually working correctly yet.

@patowen
Copy link
Collaborator

patowen commented Aug 9, 2024

I wanted to provide this comment mainly out of curiosity:

Weirdly, when I tried to compile this PR, I got

error[E0282]: type annotations needed for `Box<_>`
  --> C:\Users\Patrick\.cargo\registry\src\index.crates.io-6f17d22bba15001f\time-0.3.28\src\format_description\parse\mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items
   |              ++++++++

I resolved this by deleting Cargo.lock, so if anyone is interested in investigating what semver issue lead to this error, I've attached the Cargo.lock file that caused the error:

Cargo.lock.zip

It's probably not worthwhile debugging, though. It seems to be entirely unrelated to this PR and likely instead related to changes to my setup from hopping around various PRs.

@Ralith
Copy link
Owner Author

Ralith commented Aug 9, 2024

Probably a minor issue in an upstream crate that got fixed.

client/src/main.rs Outdated Show resolved Hide resolved
@patowen
Copy link
Collaborator

patowen commented Aug 11, 2024

This improved efficiency seems to have revealed a subtle bug I introduced in https://github.com/Ralith/hypermine/pull/414/files#diff-e8d8336a10bad73657026c14d2e141ca92d15c95a92aa7ca93df5366de21754eR186.

Normally, the server's graph is kept in a clean state. Whenever nodes are added to it, these nodes are processed fully before any other code gets a chance to run. See code snippet. This generally creates a guarantee that if snapshot is called (for transmitting info to new clients), and step is called afterwards, the Spawns returned by step will never contain any graph nodes that were already returned by snapshot.

Before a server's first step, its state is polluted. Calls to ensure_neighbor are invoked without an associated populate_fresh_nodes. This means that when snapshot is called, these nodes are included as if they're already initialized. However, step still views these nodes as new/uninitialized, so it also includes them in the Spawns it returns.

The effect is that if a client connects to a server before the server runs its first step, then as long as there are any players outside of the root node that need to be loaded, the server will send a client the same node twice, resulting in a crash.

The simplest workaround is to add result.step(); right before result is returned in Sim::new, guaranteeing that the first step will complete before anything else.

However, a longer-term fix would be to ensure that server::Sim never reaches this invalid state to begin with. I'm open to ideas on how to do this. Long-term, I wonder if we want to stop Graph from storing its own fresh nodes, since that feels like a potential foot-gun.

EDIT: This should be fixed by #427.

@Ralith Ralith force-pushed the streamlined-server branch from fa8fd23 to a3abc58 Compare August 11, 2024 17:37
@Ralith Ralith marked this pull request as ready for review August 11, 2024 17:38
@Ralith Ralith force-pushed the streamlined-server branch from a3abc58 to cf67bfa Compare August 11, 2024 17:38
@Ralith
Copy link
Owner Author

Ralith commented Aug 11, 2024

Long-term, I wonder if we want to stop Graph from storing its own fresh nodes, since that feels like a potential foot-gun.

Our plan has always been to do away with the "fresh nodes" paradigm entirely in favor of deterministic node IDs, right?

@patowen
Copy link
Collaborator

patowen commented Aug 11, 2024

Our plan has always been to do away with the "fresh nodes" paradigm entirely in favor of deterministic node IDs, right?

I believe so, although it's unclear to me what that will look like. It probably makes sense to figure that out before deciding where to store them.

@Ralith Ralith force-pushed the streamlined-server branch from cf67bfa to bb0c497 Compare August 12, 2024 00:24
@Ralith Ralith force-pushed the streamlined-server branch from bb0c497 to 5115bb3 Compare August 12, 2024 00:25
@Ralith Ralith enabled auto-merge (rebase) August 12, 2024 00:25
@Ralith Ralith merged commit a3b5cc7 into master Aug 12, 2024
4 checks passed
@Ralith Ralith deleted the streamlined-server branch August 12, 2024 00:35
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.

2 participants