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

bug: DurableObjectId::id.name doesn't work #2240

Open
threepointone opened this issue Jun 8, 2024 · 7 comments
Open

bug: DurableObjectId::id.name doesn't work #2240

threepointone opened this issue Jun 8, 2024 · 7 comments

Comments

@threepointone
Copy link

(maybe this is a feature request?)

When creating a durable object with .idFromName(name), I would expect this.ctx.id.name inside a durable object. to reflect the same. The types imply that it should it work, was it just missed?

We use this name extensively, but currently have to hack it in by passing it in with an explicit method, but it's super cumbersome to do so without a bunch of hacks and making sure it's passed before any subsequent requests/methods are called. It would simplify a lot if workerd/workers/wrangler could provide it by default. Please and thank you!

@threepointone
Copy link
Author

The "hack"/workaround we have also doesn't work for alarms, so fixing this would fix that bug as well.

@kentonv
Copy link
Member

kentonv commented Jun 9, 2024

This bothers me too, but I can't figure out how to make it work. The thing is, you are allowed to do:

let id = ns.idFromName("foo");
let id2 = ns.idFromString(id.toString());
ns.get(id2);

In this case, we've lost the name, because id.toString() only encodes the hex ID, not the name. How do we support this?

Moreover, for alarms, where does the name get stored? Does everything in the system which stores a DO ID need to store the name too, or do we implicitly store the name in the DO's own metadata?

Storing it in the metadata seems highly preferable (I guess it would get initialized the first time a name-bearing ID is used?), but then this gets weird when you consider that DOs are implicitly deleted if they shut down while their storage is empty. So the name has to be forgotten at that point. But only if the storage is empty? Is that weird?

@threepointone
Copy link
Author

I was meaning to ask, why is that pattern even around? I kinda remember it being in the docs, but can't find it there anymore (and rewrote some of my non-released code to not have that intermediate step anymore).

I think we can simply message the conditions under which that name will exist.

  • It won't exist if using newUniqueId()
  • it won't exist if using the above pattern (or rather, it'll return the hex id instead)
  • it's only ns.get(ns.idForName(...)). This might exclude any existing DOs that rewrite to this pattern, but that's a hit we should take

I guess it would get initialized the first time a name-bearing ID is used?)

Yes

his gets weird when you consider that DOs are implicitly deleted if they shut down while their storage is empty. So the name has to be forgotten at that point. But only if the storage is empty? Is that weird?

Oh hmm. Does that mean it won't be available in alarms? In which case, can we store the name everywhere the ID is stored?
Or does setting an alarm mean that storage isn't empty? In which case, it's fine, since it'll get the name again when it comes back to life.

@kentonv
Copy link
Member

kentonv commented Jun 9, 2024

The next alarm time counts as a stored value, so when an alarm is present then storage is not empty.

There are other built-in features planned which are likely to store hex IDs and where we wouldn't really want those systems to have to think about storing names alongside them.

But perhaps we can be reasonably confident that future features similar to alarms that store a hex ID only are never going to be waking up an empty object? And therefore the only way an object might not know its name is if application code wakes up an empty object by ID? Maybe that's OK?

Another weird issue: Technically someone could be passing in a multi-megabyte string as a name, which works fine if it's just getting hashed down but not so fine if we have to transmit that name over the network implicitly. Maybe there's some threshold at which we refuse to automatically store the name for them?

@threepointone
Copy link
Author

threepointone commented Jun 10, 2024

And therefore the only way an object might not know its name is if application code wakes up an empty object by ID? Maybe that's OK?

Yeah, I think that's ok too

Maybe there's some threshold at which we refuse to automatically store the name for them?

Also ok! Maybe we disallow giant names at all, unless there's a usecase I don't know.

@vy-ton
Copy link

vy-ton commented Jun 14, 2024

for when @joshthoward is back, maybe a good first issue for July onboarding folks

@rtbenfield
Copy link

We have a common pattern in Prisma where we name DOs based on some value that we also need inside the DO. One such example is our tenant ID which is a value created by newUniqueId() of one DO initially and then used to as the name of other DO classes that are spawned from that tenant (often from additional workflows). Those spawned DOs often need their tenant ID value to do their work, so we pass it in on an initialization call and put it into storage. This is a bit awkward as we need to forward the tenant ID with any calls into the spawned DO just in case it is the first time it is being initialized for that tenant.

RpcTarget is useful here as we can improve the interaction with these DOs by having a single RPC method for initializing with a tenant ID (or other context) which returns an RpcTarget implementation containing the actual operations. This ensures that we provide the appropriate initialization context while not having to awkwardly include it as part of every method or HTTP request on the DO. I suppose it's the same end result, though it feels much nicer to work with.

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

No branches or pull requests

4 participants