Handle exceptions during update so that they do not break subsequent updates #607

sorvell · 2019-03-12T17:38:39Z

Fixes #262.

Catch exceptions during update and ensure the element marks itself as finished updating.

Note, this change also ensures the updateComplete promise is rejected if there's an exception during update.

…updates Fixes #262. Catch exceptions during update and ensure the element marks itself as finished updating. Note, this change also ensures the `updateComplete` promise is rejected if there's an exception during update.

justinfagnani · 2019-03-13T18:47:45Z

src/lib/updating-element.ts

+    let result = null;
+    // Trap errors in updating so that element is not in a non-functional state.
+    try {
+      // Allow `performUpdate` to be asynchronous to enable scheduling of updates.


I don't immediately see how the comment relates to the code. What part of the statement allows performUpdate to be async?

justinfagnani · 2019-03-13T18:50:49Z

src/lib/updating-element.ts

    // Note, this is to avoid delaying an additional microtask unless we need
    // to.
    if (result != null &&
        typeof (result as PromiseLike<unknown>).then === 'function') {
-      await result;
+      try {


Can you merge with the previous try/catch block?

try { const result = this.performUpdate(); if (result != null && typeof (result as PromiseLike<unknown>).then === 'function') { await result; } } catch (e) { this._markUpdated(); reject(e); }

Done, thanks.

src/lib/updating-element.ts

justinfagnani · 2019-03-13T21:20:24Z

@sorvell should we update any documentation for updateComplete or requestUpdate to mention that they could reject?

sorvell · 2019-03-13T21:28:19Z

Added https://github.com/Polymer/lit-element/issues/608 to deal with the documentation.

kevinpschaaf · 2019-03-13T21:49:07Z

src/lib/updating-element.ts

+      }
+    } catch (e) {
+      // Ensure subsequent updates are ok but reject this update.
+      this._markUpdated();


_markUpdated is called in performUpdate after update and before firstUpdated; if firstUpdated or updated throws, _markUpdated is called twice. Is that ok? That would potentially throw out property queued changes before the error was thrown. Care?

sorvell · 2019-03-13T22:39:18Z

A user error could occur in any of the following overridden spots:

performUpdate
- shouldUpdate
- update
- firstUpdated
- updated

After update, the element should clear the update state. Given this, it makes sense to try/catch shouldUpdate + update to ensure that invariant.

Clearing the update state means the element (a) marks itself as updated, allowing additional updates to be enqueued; and (b) clears any changed properties.

It's important to note that firstUpdated/updated can enqueue an additional update.

If performUpdate throws, it may be before or after a call to super or super may not be called. In that case it's hard to decide what state this should leave the element in. It should definitely do (a) but it's unclear if it should do (b). We could set a didUpdate state in update and only do (b) if that is not set. Or, we could expose the currently private _markUpdated() and require overrides of performUpdate to handle exceptions and ensure this method is called. It's unclear any of this is worth the trouble, however.

justinfagnani · 2019-03-14T22:35:48Z

Offline review: catching shouldUpdate and update directly in performUpdate is better so we can ensure _markUpdated is called exactly once. Add documentation to performUpdate to say that any override must call super.performUpdate, even in the case of an exception in the override.

justinfagnani

flipping approve bit off pending the new updates

* `shouldUpdate`/`update` are try/caught so that the element state is ok for further updates if an exception is thrown * `firstUpdated`/`updated` are not called if there's an update exception * `performUpdate` is try/caught only to reject the update promise. This is done to handle an override producing an exception. * a private version `_requestUpdate` is called in the property setter to avoid accessing the overridable `updateComplete` promise. * Added additional js doc comments.

kevinpschaaf · 2019-03-14T23:19:36Z

src/lib/updating-element.ts

+      // enable coordinating updates with a scheduler. Note, the result type is
+      // checked to avoid delaying an additional microtask unless we need to.
+      if (result != null &&
+          typeof (result as PromiseLike<unknown>).then === 'function') {


This code seems superfulous

kevinpschaaf

LGTM

sorvell requested review from justinfagnani and kevinpschaaf March 12, 2019 17:38

googlebot added the cla: yes label Mar 12, 2019

update tests consistently to use helper to add/remove test elements

bae7bf7

sorvell added this to the 2.0.x milestone Mar 12, 2019

Fix lint error

c28f5f3

justinfagnani requested changes Mar 13, 2019

View reviewed changes

Address review feedback

cc99a49

justinfagnani approved these changes Mar 13, 2019

View reviewed changes

src/lib/updating-element.ts Show resolved Hide resolved

src/lib/updating-element.ts Show resolved Hide resolved

Address review feedback.

919a01a

kevinpschaaf reviewed Mar 13, 2019

View reviewed changes

justinfagnani requested changes Mar 14, 2019

View reviewed changes

kevinpschaaf reviewed Mar 14, 2019

View reviewed changes

kevinpschaaf approved these changes Mar 14, 2019

View reviewed changes

Steven Orvell added 2 commits March 14, 2019 16:42

Simplify promise check and fix lint errors

b1dffa5

Merge from master

bb2f871

justinfagnani approved these changes Mar 15, 2019

View reviewed changes

kevinpschaaf merged commit 3fb5cda into master Mar 15, 2019

kevinpschaaf deleted the update-exceptions branch March 15, 2019 00:49

sorvell mentioned this pull request Jan 12, 2022

Document handling rejected updateComplete promise lit/lit.dev#649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle exceptions during update so that they do not break subsequent updates #607

Handle exceptions during update so that they do not break subsequent updates #607

sorvell commented Mar 12, 2019 •

edited

Loading

justinfagnani Mar 13, 2019

sorvell Mar 13, 2019

justinfagnani Mar 13, 2019

sorvell Mar 13, 2019

justinfagnani commented Mar 13, 2019

sorvell commented Mar 13, 2019

kevinpschaaf Mar 13, 2019

sorvell commented Mar 13, 2019 •

edited

Loading

justinfagnani commented Mar 14, 2019

justinfagnani left a comment

kevinpschaaf Mar 14, 2019

kevinpschaaf left a comment

Handle exceptions during update so that they do not break subsequent updates #607

Handle exceptions during update so that they do not break subsequent updates #607

Conversation

sorvell commented Mar 12, 2019 • edited Loading

justinfagnani Mar 13, 2019

Choose a reason for hiding this comment

sorvell Mar 13, 2019

Choose a reason for hiding this comment

justinfagnani Mar 13, 2019

Choose a reason for hiding this comment

sorvell Mar 13, 2019

Choose a reason for hiding this comment

justinfagnani commented Mar 13, 2019

sorvell commented Mar 13, 2019

kevinpschaaf Mar 13, 2019

Choose a reason for hiding this comment

sorvell commented Mar 13, 2019 • edited Loading

justinfagnani commented Mar 14, 2019

justinfagnani left a comment

Choose a reason for hiding this comment

kevinpschaaf Mar 14, 2019

Choose a reason for hiding this comment

kevinpschaaf left a comment

Choose a reason for hiding this comment

sorvell commented Mar 12, 2019 •

edited

Loading

sorvell commented Mar 13, 2019 •

edited

Loading