Skip to content

Provide a way to determine if a durable object is broken from errors#151

Merged
bcaimano merged 1 commit intomainfrom
bcaimano/do-broken-errors
Nov 21, 2022
Merged

Provide a way to determine if a durable object is broken from errors#151
bcaimano merged 1 commit intomainfrom
bcaimano/do-broken-errors

Conversation

@bcaimano
Copy link
Contributor

@bcaimano bcaimano commented Nov 3, 2022

Currently, workers have to rely upon the Error.message value to determine if their durable object is broken. Usually, the worker will want to retry their request to reinitialize their durable object and continue work. Thus we want to set a property like the existing Error.remote property to convey that the the request failed because the durable object became broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since we aren't modifying isDurableObjectBroken and it's a primitive, it feels like we should pass a value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh, good point! And it should all inline better if the lambda only has arguments anyway.

Copy link
Contributor

@MellowYarker MellowYarker left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, I remember R2 had mentioned wanting this!

@bcaimano bcaimano force-pushed the bcaimano/do-broken-errors branch from e6a1961 to 9ea31c4 Compare November 3, 2022 19:20
return jsg::check(
obj->Set(
isolate->GetCurrentContext(),
jsg::v8Str(isolate, "isDurableObjectReset"_kj, v8::NewStringType::kInternalized),
Copy link
Member

Choose a reason for hiding this comment

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

The is prefix here seems a bit inconsistent with the .remote field (i.e. it's .remote, not .isRemote). I don't feel super strongly, but I think just .durableObjectReset would match better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmmm, I'd be curious as to @jasnell 's opinion here. I considered naming parity with .remote but .durableObjectReset didn't feel like a property name for a boolean to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @a-robinson here. I'm not a fan of the is* prefix on getters for JavaScript types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it over!

@bcaimano bcaimano force-pushed the bcaimano/do-broken-errors branch from 9ea31c4 to d7f0388 Compare November 17, 2022 21:57
Currently, workers have to rely upon the `Error.message` value to determine if their durable object is reset. Usually, the worker will want to retry their request to reinitialize their durable object and continue work. Thus we want to set a property like the existing `Error.remote` property to convey that the the request failed because the durable object became reset.
@bcaimano bcaimano force-pushed the bcaimano/do-broken-errors branch from d7f0388 to 7ff63aa Compare November 17, 2022 21:58
@bcaimano bcaimano merged commit f2a89f4 into main Nov 21, 2022
@kentonv kentonv deleted the bcaimano/do-broken-errors branch December 7, 2022 03:01
@kentonv
Copy link
Member

kentonv commented Dec 7, 2022

If we had the more general retryable property, would there be a reason for durableObjectReset to remain as a distinct property or could it have been folded into retryable?

@bcaimano
Copy link
Contributor Author

bcaimano commented Dec 9, 2022

If we had the more general retryable property, would there be a reason for durableObjectReset to remain as a distinct property or could it have been folded into retryable?

Yes, but it would have been a not so great reason. :)

durableObjectReset is a subset of retriable in my view. If you do not depend upon the in-memory state of the DO to serve the request, then durableObjectReset = true means just retry. That said, I can envision someone trying to do something like this (as much as I'd rather they not):

  • Worker sends request A containing large blob and a session id that they want the DO to do something with.
  • DO stores the blob in memory according to a key, but returns a response saying "I need more info".
  • Worker fetches the info from somewhere using credentials not exposed to the DO.
  • Worker sends request B with the additional info and the session id.

If request B is rejected with durableObjectReset = true, the worker knows it needs to start over with request A (and unfortunately send the large blob again). Without durableObjectReset, their best option is to retry request B, get told that the session is unknown, and then start over with A. This can be mitigated by putting the large blob in durable object storage, but that potentially means additional latency, storage usage, and complexity. Not everybody will want to make that trade off.

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.

5 participants