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

Propagate errors from indexedDB database.put #7322

Merged

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Jan 28, 2023

database.put can sometimes throw an error if the data being stored is too large. Propagate that error to the promise instead of letting it escape the callback.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

database.put can sometimes throw an error if the data being stored is too
large. Propagate that error to the promise instead of letting it escape the
callback.
@mattsoulanille
Copy link
Member Author

mattsoulanille commented Jan 28, 2023

I tried testing this by spying on indexedDB, but it returns native objects, which I can't spy on. The correct approach would be to write a mock, but that would be a lot of work. Let me know if you want me to do that.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn and @mattsoulanille)


tfjs-core/src/io/indexed_db.ts line 170 at r1 (raw file):

              infoStore.put({modelPath: this.modelPath, modelArtifactsInfo});
          } catch (error) {
            reject(error);

should it be, also for the other change
return reject(error);

Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mattsoulanille
Copy link
Member Author

@pyu10055 Please take a look. Thanks!

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @mattsoulanille)

@mattsoulanille mattsoulanille merged commit 319720c into tensorflow:master Feb 20, 2023
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.

3 participants