fix errors related to local storage quota #141

jhump · 2021-10-07T02:05:41Z

Specifically:

Use less local storage: don't store entire RPC response since it's not even used. Now we only store the minimum response info for rendering item in history.
We limit the data we'll write to a single key to 1mb. Most grpcui instances will have only one key, so this should keep us well under normal quota (which is 5mb).
Even after above limit, if we still hit quota exception: don't blow up! Instead, log the error and keep going.

This also includes a couple of other minor fixes for things I identified while testing out the history changes/fixes (see comments below for more details).

Resolves #124.

- don't store entire RPC response since it's not even used; only store the minimum response info for rendering item in history - limit storage for a single key to 1mb -- most grpcui instances will have only one key, so this should keep us well under normal quota (which is 5mb) - even after above limit, if we still hit quota issue: don't blow up! instead, log the error and keep going

jhump · 2021-10-07T02:07:12Z

internal/resources/webform/webform.js

+    $("#grpc-service").change(() => formServiceSelected());
+    $("#grpc-method").change(() => formMethodSelected());


This is one of the extra fixes. I kept noticing a nuisance "TypeError" in the console. It's because when these functions were invoked, the machinery was passing an arg that was not a callback. But these functions expect to either get zero args or one arg that is a callback function. So trying to "invoke" the one arg, as if it were a function, would fail and result in console noise.

jhump · 2021-10-07T02:07:40Z

testing/cmd/testsvr/testsvr.go

 	}

-	var m *TestMessage
+	var lastReq *TestMessage


This is the other extra fix: this UploadMany test RPC would always fail with "invalid argument", thinking it had never received any messages.

jhump · 2021-10-07T02:10:31Z

cc @ianrose14, who has also mentioned this error to me before

dragonsinth · 2021-10-07T15:03:04Z

Go code LGTM but maybe we could still get @gtg471h to review the javascript stuff?

jhump · 2021-10-07T20:43:53Z

@jameremo, do you mind reviewing some JS code?

dragonsinth · 2021-10-07T21:37:05Z

Chris said he could, just not immediately

jameremo · 2021-10-08T12:33:49Z

I'm not sure it's fair to ask Chris to keep contributing. Of course his input is always welcome (we miss you, man), but I'll spend some time this week coming up to speed on the JS code as a more long-term plan for our team.

jhump · 2021-10-08T12:49:52Z

I'm not sure it's fair to ask Chris to keep contributing.

Agreed. I also don't like the idea of having to wait on external contributors, who can't be realistically expected to prioritize this, in order to land fixes to stuff that we own.

gtg471h · 2021-10-09T14:03:44Z

internal/resources/webform/webform.js

+        } catch (e) {
+            // Likely no room in local storage quota. This can still happen, despite
+            // above code to limit the size, because there could be other keys in
+            // storage OR we could be in incognito mode, which doesn't allow writing


Note: this only appears to affect mobile safari as far as the incognito issue is concerned.

in Mobile Safari (since iOS 5) it always throws when the user enters private mode. (Safari sets the quota to 0 bytes in private mode, unlike other browsers, which allow storage in private mode using separate data containers.)

gtg471h · 2021-10-09T14:05:10Z

internal/resources/webform/webform.js

+        }
+
+        try {
+            localStorage.setItem(storageKey, data);


Should the code attempt to shave more history if an error occurs? Even if incognito mode causes issues, you would only shave the current one... Not sure how what happens if you attempt to store an empty string but you could break if the data length is 0.

Not sure how what happens if you attempt to store an empty string but you could break if the data length is 0

I think data will never be a 0-length string; it's the result of stringifying an array, so even if the array is empty, the stringified value will still be "[]".

Should the code attempt to shave more history if an error occurs?

I think the for loop above, which shaves history until we're below 1 Mb, is a good starting compromise. It's not clear that shaving more history after that would help, either in the incognito case or in the multi-key case. For the latter, I think users who are running multiple instances on the same domain will need to clear history on several or all their domains. Maybe that's worth putting a note about in #124.

Now that we're not storing the entirety of the response, and assuming old history items are cleared, it seems unlikely that 100 history items will even come close to 1 Mb. I guess really large requests could still cause the history to be large, but I don't think gRPCui is the right tool to make such requests, at least not in its present state (I don't see that it lets you upload files, so you'd have to type or paste in that enormous content).

I thought for a minute about whether limiting it to 20% of the storage per domain is too much? Like, is it impolite for one app to presume to use that much? But based on our own internal uses cases at FullStory, this is very reasonable. We can wait to see if the community has issues with this limit.

Should the code attempt to shave more history if an error occurs?

@gtg471h: howdy, Chris!!! Hope you are doing well! Thanks for taking a look!

I think that is overkill. If the thing we think should work doesn't, I think it's okay to degrade to a state where we're not persisting history and logging an error to the console. If this does happen, maybe we'll get a bug report and can figure out the best fix at that time.

20% of the storage per domain

FYI, it's not per domain. Every domain gets its own local storage -- that's a browser security feature to prevent code from different origins from interacting/interfering with one another.

So it's actually per target. Like if you use the command-line tool repeatedly and set the port, then you'll have a fixed domain/origin (e.g. localhost:12345) that can store history for multiple targets, based on the server address you provide. For embedded gRPC UIs or servers/containers in the cloud, it's most likely to always be exactly one target.

I thought about actually making this more sophisticated -- like having it load all of the keys and then start removing the oldest item across everything stored until all of storage is small enough to fit. But that seemed like serious overkill. I think we could wait to add that level of smarts until people actually run into issues with this approach (which may be never). This approach, with a limit of 100 requests and no longer storing responses, is likely to be fine, even if someone has history for over a 100 targets.

So I'm exposing my dreadful ignorance of local storage here, but can you explain more what you mean by "target"? The Mozilla docs say that you can access a storage object "for the Document's origin", and then it defines origin as a combination of scheme, hostname, and port. I interpreted that to mean that the browser used the same storage object for all documents on the same origin.

Sorry -- target in this sense relates to the grpcui command-line tool: you can run it and point it at "localhost:12345" or "api.grpc.me:443", etc. These are all different targets, and grpcui stores the history for each target as a separate key in local storage. So a single domain (like if you served grpcui from "localhost:22222") could have multiple keys in its local storage, one for each such target.

jameremo

I certainly can't call myself a JS expert, but I agree with the proposed solution for limiting history size, and don't have any language-specific nits.

LGTM

jameremo · 2021-10-11T13:25:46Z

internal/resources/webform/webform.js

+        }
+
+        try {
+            localStorage.setItem(storageKey, data);


Not sure how what happens if you attempt to store an empty string but you could break if the data length is 0

I think data will never be a 0-length string; it's the result of stringifying an array, so even if the array is empty, the stringified value will still be "[]".

Should the code attempt to shave more history if an error occurs?

I think the for loop above, which shaves history until we're below 1 Mb, is a good starting compromise. It's not clear that shaving more history after that would help, either in the incognito case or in the multi-key case. For the latter, I think users who are running multiple instances on the same domain will need to clear history on several or all their domains. Maybe that's worth putting a note about in #124.

Now that we're not storing the entirety of the response, and assuming old history items are cleared, it seems unlikely that 100 history items will even come close to 1 Mb. I guess really large requests could still cause the history to be large, but I don't think gRPCui is the right tool to make such requests, at least not in its present state (I don't see that it lets you upload files, so you'd have to type or paste in that enormous content).

I thought for a minute about whether limiting it to 20% of the storage per domain is too much? Like, is it impolite for one app to presume to use that much? But based on our own internal uses cases at FullStory, this is very reasonable. We can wait to see if the community has issues with this limit.

jhump force-pushed the jh/fix-storage-quota-issue branch from 51f7969 to 6d73757 Compare October 7, 2021 02:09

jhump commented Oct 7, 2021

View reviewed changes

jhump assigned dragonsinth Oct 7, 2021

jhump unassigned dragonsinth Oct 7, 2021

jhump assigned jameremo Oct 7, 2021

gtg471h reviewed Oct 9, 2021

View reviewed changes

jameremo approved these changes Oct 11, 2021

View reviewed changes

jhump merged commit 461f499 into master Oct 12, 2021

jhump deleted the jh/fix-storage-quota-issue branch October 12, 2021 12:37

		$("#grpc-service").change(() => formServiceSelected());
		$("#grpc-method").change(() => formMethodSelected());

fix errors related to local storage quota #141

fix errors related to local storage quota #141

Uh oh!

Conversation

jhump commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhump commented Oct 7, 2021

Uh oh!

dragonsinth commented Oct 7, 2021

Uh oh!

jhump commented Oct 7, 2021

Uh oh!

dragonsinth commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jameremo commented Oct 8, 2021

Uh oh!

jhump commented Oct 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhump Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameremo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jhump commented Oct 7, 2021 •

edited

Loading

dragonsinth commented Oct 7, 2021 •

edited

Loading

jhump Oct 12, 2021 •

edited

Loading