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

forceBypassCache param removed #468

Merged
merged 41 commits into from Nov 14, 2019
Merged

Conversation

@mspalti
Copy link
Contributor

mspalti commented Aug 29, 2019

All references are (or should be) gone and the requestService is modified.

Fixes #394

Also, I had an unrelated problem with the unit test for date lists. Although it had passed previously, the 1950-01-01 full year was being translated to 1949 in my environment. I changed the method call from getFullYear() to getUTCFullYear() and the problem was solved. I left the change here in the code with a TODO note.

Copy link
Member

artlowel left a comment

I would expect that, wherever forceBypassCache used to be true, you would now have to set responseMsToLive on the Request, as that is what determines how long something is cached for. So for example this GetRequest which used to use forceByPassCache, should now have its responseMsToLive property set to 0.

For the submissionRequests it may be possible to cache them for a short time, or perhaps even the default time. It needs to be judged on a case by case basis.

Although I guess you could also set them all to 0 for this PR, that way the forceByPass param is removed, and then create a new ticket that lists the requests that used to use it, saying that we need to re-evaluate wether or not they benefit from caching.

@mspalti mspalti force-pushed the mspalti:forceBypassCache_remove branch from f7b93b0 to beaf890 Sep 16, 2019
@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Sep 16, 2019

@artlowel , when the forceBypassCache had been true (in dataService, authRequest) I set the responseMsToLive property to 0 as you suggest.

For subclasses (e.g. WorkflowItemDataService) that had previously set forceBypassCache on the dataService, I introduced a new property on dataService and then set the value of that new property in the subclasses. The value is set to 0 in call cases. Let me know if you'd prefer to just set responseMsToLive to 0 in dataService for now since it's not clear that something like this property will be needed.

src/app/core/data/data.service.ts Outdated Show resolved Hide resolved
src/app/core/data/data.service.ts Outdated Show resolved Hide resolved
Copy link
Member

artlowel left a comment

Thanks @mspalti!

One last minor comment: There's still one unused forceBypassCache variable that could be removed

Copy link
Contributor

atarix83 left a comment

@mspalti thanks for the PR

I made some tests and I occurred on an issue, that doesn't occur on master branch.
When from the mydspace page I try to edit an in progress submission by clicking on edit button I'm redirecting to workspace edit page properly, but the loading spinner is hanging forever
Schermata del 2019-10-03 14-28-09
Schermata del 2019-10-03 14-34-51

I suspect that the problem is on RequestService's requestsOnTheirWayToTheStore array.
here this method was used to clean up the request completed from the requestsOnTheirWayToTheStore array otherwise the get requests weren't execute. If this is the cause we should find a way to remove the completed request from the array.

@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Oct 3, 2019

It shouldn't be necessary to ever manually clear those requests.

The reason requestsOnTheirWayToTheStore exists, is because a request doesn't immediately arrive in the store as soon as you dispatch the action. The store processes these actions asynchronously and that can take a few ms. However, when deciding whether a new request to a URL is already pending, we need to check it against the ones that haven't reached the store as well.

So when the requestService dispatches a new RequestConfigureAction, the trackRequestsOnTheirWayToTheStore adds that request to a local array and immediately after selects the place in the store where that request will be put when the store processes the action. It subscribes to that observable to remove the request from the local array when it turns up in the store.

I don't see any reason to ever manually clear that array. If two submission requests to the exact same URL happen in such quick succession that the first request hasn't even reached the store when the second arrives, we should just ignore the second request and await the response to the first.

@mspalti mspalti force-pushed the mspalti:forceBypassCache_remove branch from d5e600c to 604e99e Oct 3, 2019
this.clearRequestsOnTheirWayToTheStore(request);
}
if (!isGetRequest || (forceBypassCache && !this.isPending(request)) || !this.isCachedOrPending(request)) {
const isSubmission = request instanceof SubmissionRequest;

This comment has been minimized.

Copy link
@mspalti

mspalti Oct 3, 2019

Author Contributor

This might not be the right solution, but it does appear to fix the problem noticed by @atarix83

I think previously the combination of forceBypassCache and removing pending requests from requestsOnTheirWayToTheStore had the effect of forcing a new dispatch request. Without this, a lookup by href is attempted and fails. Checking for the request type is a different way to get the same result but may have unintended consequences.

Also, I noticed that SubmissionService and RemoteDataBuildService are both importing methods from the shared/testing/utils It seems like those methods would be better located in a different utility.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 4, 2019

Now that see what's causing the problem I'm wondering about this suggestion by @artlowel

first request hasn't even reached the store when the second arrives, we should just ignore the second request and await the response to the first.

That's sort of what I did here but I think at the cost of dispatching a second and (maybe!) unnecessaryRequestExecuteAction. I'll look at this tomorrow. If either of you see this before feel free to make suggestions.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 5, 2019

I looked at this more. From what I saw, I don't think the issue is related to pending requests in requestsOnTheirWayToTheStore but there do seem to be issues with the cache.

I still don't fully understand the data cache, but it's clear that simply calling RequestService.getByHref and dispatching an AddToIndexAction fails because there's no href workspace entry in the get-request/href-to-uuid cache (even when data for the workspace item does exist in cache/object).

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 5, 2019

Also RequestService.isCachedOrPending correctly returns true because the entry exists in the object/cache. But currently this should be ignored for submissions.

Presumably the cached data will be used if we "forceBypassCache" by other means, but I'm still not clear on why this is necessary in the case of workspace submission requests.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 6, 2019

OK. I think I see the reason why bypassing the cache is necessary (for example by adding something like isSubmission to RequestService.configure). In the current state there's no entry in data/request. But an entry is required for the lookup in SubmissionRestService.fetchRequest. This entry is created when we bypass cache and dispatch new configure and execute actions. That forces a new REST api request.

It's a "new" request because data from prior workspaceitem requests does exist in the object cache and is ignored. I'm certain that is by design. It's probably a good idea to always fetch workspace item data from the server, in case something has changed.

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Oct 7, 2019

So the situation is that the workspaceitem is present in cache/object because it's retrieved previously while loading the mydspace page. When requesting the workspaceitem by id there is no response because there is no entry in data/request.
So regardless if a new request is dispatched or not, we should get a response, I think this is a bug to fix.

maybe @artlowel could have suggestions in this regard

@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Oct 7, 2019

I took a look in the debugger, and the reason for the issue @atarix83 reported seems to be that the workspace item is cached in the object cache because it was retrieved for the mydspace page. It is cached for the default duration of 15 minutes: the object cache msToLive doesn't inherit the responseMsToLive time, and that's a bug.

I fixed it in this commit (The branch started from this PR). Once I did that though I also had to increase the places where responseMsToLive was 0 to 10 seconds (lowering it further could work as well, haven't tested it) because otherwise I got stuck in an infinite loop of requests being removed from the cache before they could be used, and being re-requested.

I don't think it makes a lot of sense releasing this as a separate PR against master, as it will affect a lot of the same files and conflict with this PR. So I'd say @mspalti, take a look at my branch, and if you're happy it works and solves your issues, merge it with yours and push it to this PR.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 7, 2019

@artlowel I reviewed your fix and merged it with this PR. I reduced the responseMsToLive values to 5 seconds, and then to 2 seconds. Both worked, somewhat. But if I navigated between mydspace and submissions I seemed get into a loop on occasion with 2 seconds. Possibly with 5 as well. It was a bit difficult to test because problems were intermittent.

Also, at 10 seconds it appeared that I could recreate the original problem that @atarix83 noticed if I was quick to select an item after first navigating to mydspace.

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Oct 8, 2019

@artlowel thanks for fix
@mspalti i think that if can you reduce responseMsToLive to 1 sencond here and here so there should not be other issues

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 8, 2019

@atarix83 unfortunately when I reduce times I see problems. I just tried reducing the times as you suggested and mydspace stopped working for me. I see a loop in which requests are repeated; eventually the client stops responding. Do you see the same thing?

@mspalti mspalti force-pushed the mspalti:forceBypassCache_remove branch from b8c8b42 to ef4ae44 Oct 17, 2019
@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Oct 17, 2019

I think one way forward would be to retain forceBypassCache in RestRequest and set it to false. In cases like this one, in which the default caching logic must be bypassed and we need to guarantee that a new request always happens, we could override the forceBypassCache property as needed, for example in SubmissionRequest. That still largely removes forceBypassCache from the rest of the code.

Then create a new issue and look for a different solution if we need to.

I agree with that approach.

I wouldn't block this PR because it improves upon the previous way of working in that the forceBypassCache property is more contained, that responseMsToLive is used instead in a lot of places, and that responseMsToLive is also respected by the object cache.

So in my opinion it can be merged as-is.

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Oct 18, 2019

I would like to state that I agree with the proposed solution, but I had a deep test on this PR and unfortunately I think it can't be merged as is.

Indeed the mydspace page doesn't work properly because the issue that I reported here still occurs.

I've tried to add forceBypassCache property here but the result is that it doesn't work at all because the page stays hanging.

Additionally I also noticed that when click on workspaceitem edit the loading spinner is hanging for several seconds before to load data.

So at this state I can't approve this PR.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 19, 2019

@atarix83 , right now I'm testing against the demo REST api at 4Science. It has many collections, and that seems to dramatically affect angular client performance. That said, I tried running against the master branch for comparison with this PR. I could not detect any differences between in performance or results between master and the PR branch (which includes recent updates to master).

It might be better if I test against a smaller, local dspace instance but I can't do that at the moment (I'm travelling and using a laptop). But this does make me wonder if some of the problems here might be related to changes elsewhere.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 22, 2019

@atarix83 , once I got back to my test dspace instance I think saw the problem that you discovered. For example, I deleted a workspaceitem from the list immediately after the mydspace page loaded. The delete request was processed. But after that happened, the item was still in the list.

I tried modifying responseMsToLive and forceBypassCache in several places, and as you noted this doesn't seem to work.

What seems to work is emptying the object cache in MyDSpacePageComponent ngOnInit(), before the SearchService is called. That assures that a fresh list is requested every time the component is initialized.

@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Oct 24, 2019

Emptying the affected part of the cache after a delete makes sense. But in the past we've only done it inside the delete method on the dataservice rather than every time onInit. I guess we'll have to make an exception here, at least for now, because mydspace only seems to work if nothing is cached.

Please do track the subscribe though, so you can destroy it onDestroy, or use a take(1)

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Oct 29, 2019

Explicitly unsubscribing in onDestory seems better. Referring to this discussion in the code comment.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Nov 5, 2019

The e2e test failed to start on travis. The error message says incompatible chrome version.

@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Nov 5, 2019

That issue should be fixed if you merge in the latest master

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Nov 6, 2019

OK, e2e tests succeeded after merge.

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Nov 7, 2019

@mspalti thanks for the work on this issue.

your solution works properly.

However there is a better way of emptying the object cache. Indeed there is no need to clear cache every time mydspace page is load, but only after :

  • a workspaceitems is deposited or deleted. This could be done by adjusting this method.
  • an action on a task is performed (claim, return to pool, approve, reject) this could be done here

In this way we prevent to clear cache even when a search filter is clicked.

Feel free to resolve on this PR, otherwise please create a new issue for this.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Nov 7, 2019

@atarix83 I will make the changes you suggest on this PR.

@mspalti

This comment has been minimized.

Copy link
Contributor Author

mspalti commented Nov 8, 2019

@atarix83 , I moved clearing of the object cache into the methods you identified. Code seems to be working as before. I'm happy to make minor changes based on feedback. Otherwise, this appears ready to merge.

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Nov 14, 2019

@mspalti thanks for the change I had a look again and it looks good to me.

I think that is ready to me merged now

@tdonohue tdonohue merged commit 3381d5e into DSpace:master Nov 14, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 79.647%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.