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

feat: packet handling functions #771

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

andrewgazelka
Copy link
Collaborator

@andrewgazelka andrewgazelka commented Dec 19, 2024

I think it makes a lot of sense to have the ability to add handlers for any packets. Why is this useful? There are a couple of reasons. First, we want people to have as much access as possible, as close to the server as possible. If we’re creating events that rely on ingress packets but aren't 1:1 (lose some information), it restricts the ability of the engine.

Examples where handlers could be nice:

  • Replay mod: Imagine having a handler that inspects every single packet and serializes it to disk for replay purposes. This would be excellent for capturing every player’s actions. We need a way to add a handler for this scenario. If we were designing a replay system, such a handler might need to operate on a dynamic trait—one that can handle newly added packets in the future. It would be interesting to explore this idea in future designs.

Potential global handler on top of specific handlers

I believe it could be viable to also have single handler for every packet that implements serialization—or to mandate that all packets implement serialization. Packets already implement encode and decode in the current code, so this approach would be quite natural. As the project evolves, we might want to revisit how events are handled. Perhaps providing no default events and implementing them as standalone modules would be more efficient. We could refactor the event system in the server and assign a dedicated handler for each function.

Events and lifetimes

As part of this refactoring, ensuring events are handled safely is critical. Currently, the code converts bytes to a 'static lifetime, but ideally, the handler should use bytes with a lifetime 'a, and the event sender should also produce events tied to 'a rather than 'static. This may require some changes to how decode and encode traits are defined in Valence, but I’m open to adjustments if they align with the broader design goals. Thank you.

flowchart TD
    classDef module fill:#e1f5fe,stroke:#01579b
    classDef handler fill:#fff3e0,stroke:#ff6f00
    classDef feature fill:#f3e5f5,stroke:#7b1fa2
    classDef lifetime fill:#ffebee,stroke:#c62828

    NetworkInput[Incoming Network Packets]
    GlobalHandler[Global Packet Handler]:::handler
    SerializationTrait[Serialization Trait]:::module
    SpecificHandlers[Specific Packet Handlers]:::handler
    ReplayMod[Replay Module]:::feature
    CustomHandler[Custom Event Handler]:::feature
    EventSystem[Event System]:::module
    LifetimeA[Lifetime 'a]:::lifetime
    StaticLifetime[Static Lifetime]:::lifetime

    NetworkInput --> GlobalHandler
    GlobalHandler --> SerializationTrait
    GlobalHandler --> SpecificHandlers
    
    SpecificHandlers --> ReplayMod
    SpecificHandlers --> CustomHandler
    SpecificHandlers --> EventSystem
    
    ReplayMod --> |Serializes to Disk| Storage[(Storage)]
    
    subgraph Lifetimes
        LifetimeA --> |Current Implementation| StaticLifetime
        LifetimeA --> |Proposed Implementation| EventSystem
    end

    subgraph Potential Use Cases
        ReplayMod
        CustomHandler
    end

    subgraph Handlers
        GlobalHandler
        SpecificHandlers
    end
Loading

Comment on lines 34 to 39
// transmute to bypass lifetime issue with Decode
// packet is dropped at end of scope and references in handlers are locked to the scope of the handler
let mut bytes = unsafe { std::mem::transmute(bytes) };
let mut packet = P::decode(&mut bytes)?;
// packet is always non-null, swap to NonNull::from_mut after stabilization
let ptr = unsafe { NonNull::new_unchecked(&mut packet).cast::<u8>() };
Copy link

Choose a reason for hiding this comment

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

This creates an unsafe situation where handlers receive a pointer to packet which is dropped when it goes out of scope, while the handlers may still try to access it. This leads to undefined behavior. Consider using Arc<P> to safely share ownership of the packet data between handlers, ensuring the data remains valid for the duration of handler execution. The transmute can also be avoided by properly handling lifetimes through Arc.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@andrewgazelka andrewgazelka changed the title Unsafe packet handling feat: packet handling functions Dec 22, 2024
@github-actions github-actions bot added the feat label Dec 22, 2024
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 this pull request may close these issues.

3 participants