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

v4: Attached vs Detached gateway entities #2981

Open
quinchs opened this issue Aug 12, 2024 · 0 comments
Open

v4: Attached vs Detached gateway entities #2981

quinchs opened this issue Aug 12, 2024 · 0 comments

Comments

@quinchs
Copy link
Member

quinchs commented Aug 12, 2024

Abstract

In v4's current design, gateway entities provide the following guarantees:

  • There will only be one “attached” entity per unique identifier(s)
  • Entities are only created when user code requests them
  • There is no requirement or expectation that the entity exists in cache

Because of these requirements, entities in memory are always updated via the gateway, but in some cases you may want to "detach" the entity from receiving updates to its data.

Some usecases for this:
Concurrency
If your code is data dependent and runs independently from the gateway task, you may want to ensure that the entity isn't updated while your code runs, consider the following:

public async Task OnMessageCreateAsync(GatewayMessage message)
{
    if(message.Content == "Foo")
    {
        Console.WriteLine("Got Foo");
    }

    // some long running code
    await Task.Delay(10000);
		
    if(message.Content == "Bar")
    {
        Console.WriteLine("Got Bar");
    }
}

If the OnMessageCreateAsync runs separately from the gateway task (the gateway doesn’t wait for it to finish before processing the next event), the message can be updated at any time during this functions execution, meaning the behaviour of this function is no longer predictable.

To fix this, we could indicate that we want to “detach” the message from the client, meaning it wont be updated by the dispatch events.

It’s also noted that there are other solutions to this problem, one of them is to hold a “lock” that defers update operations to that particular entity until the lock is released. A pseudo implementation could look like the following:

public async Task OnMessageCreateAsync(GatewayMessage message)
{
	using var handle = await message.PreventUpdatesAsync();
	
	// do our logic
}

where PreventUpdatesAsync wraps the keyed semaphore for the given entity, and releases it when handle is disposed. Although this means that the callee of the update function is waiting for this lock to release, which defeats the purpose of running the gateway task separately from the handler. The actual implementation would add the update operation to a queue instead, and dequeued and executed when handle is disposed.

Comparisons for before/after updates
A common pattern for update events, and ultimately what I want for v4 as well, is to provide a before and after parameter of the updated entity, for example:

public async Task OnMessageUpdated(GatewayMessage? before, GatewayMessage after)
{
	...
}

The before parameter represents the entity before the update happened, while after represents the entity after the change. With our guarantee that only one “attached” entity will exist in memory per unique identifier, this would be impossible unless the before parameter is detached, otherwise they would both represent the same data.

Goals

  • The api for “Attached” vs “Detached” should be very explicit to the user, they should know when the data they are working with is going to be updated by the gateway.
  • Each cachable entity type should have a way to check whether or not its attached to the gateway.
  • You should be able to create as many detached versions of an entity as you want.

Changes

  • Cachable entities will expose a boolean IsAttached property that indicates whether that instance is attached and therefor managed by the gateway.
    • The flag indicating the attach state will be provided via GatewayConstructionContext and consumed and exposed by the base GatewayCachableEntity class.
  • Cachable entities will expose a Detach() function that returns a new instance of that entity type, with the data coming from the current state of the entity instance
    • The underlying actor instance is reused.
    • Any outbound actors/traits are reused.
  • Attempting to get a handle to a detached handle will not be valid, and for runtime checks will throw an exception:
    • Gateway events that expose a detached entity will not support IEntityHandle as the parameter type

Design Considerations

Question

What should happens to the updateCache flag and request options for attached and detached entities when calling UpdateAsync or any function that modifies the underlying data?

Answer

For attached entities
We cant switch out the instance or detach the instance from the cache safely since there could be other functions relying on that instance, which means that when updateCache is false, the applied changes will only be accessible for the current process, until the entity is updated again. This could create unwanted behaviour like de-syncing for functions that update the state, but not the Discord API .

For detached entities
Updating the underlying cache means that all attached entities get updated too, which can create dataloss depending on how stale the detached entity is, ex:

var message = await channel.Messages[123].GetAsync();
var detachedMessage = message.Detach();

Console.WriteLine(message.Content); // prints "original content"

// do work
// during this work, lets say we received the following dispatch:
// MESSAGE_UPDATE: content="updated content"
// this updates 'message' with the new content

var someMessageModel = ...;
await detachedMessage.UpdateAsync(someMessageModel, updateCache: true);

// 'message' at this point is still attached, and our 'updateCache' has
// propigated the 'someMessageModel' to it.

Console.WriteLine(message.Content); // prints "original content"

Proposal
We could remove the updateCache flag entirely and only update the underlying cache if the entity is attached, this now implies that the cache and the attached entity is fast-forwarded to whatever the model parameter was, in most cases this will come from a Discord API call, so this will be fine.


Question

Should we have an explicit type that represents a detached entity? something like:

public interface IDetachedEntity<TId, TEntity, TModel>
{
	// the id of the entity
	TId Id { get; }
	
	// the detached entity 
	TEntity Value { get; }
	
	// the model that represents the detached versions data
	TModel Model { get; }
}

which would be used as a counter-type to IEntityHandle, meaning:

  • IEntityHandle explicitly represents an entity attached to the gateway.
  • IDetachedEntity explicitly represents an entity detached from the gateway.
  • the entity type itself can be attached or detached.

This would have clear and explicit indications for what your code is expecting data-wise, which is great for DX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant