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

make access to botCache and anonymousCache more robust #2411

Closed
wants to merge 1 commit into from

Conversation

saschaszott
Copy link
Contributor

Both botCache and anonymousCache are only initialized if the Angular app is running in production mode. This means that it is not sufficient to check the return value of botCacheEnabled() / anonymousCacheEnabled() is true.

If you're running in development mode you'll get a TypeError (access on undefined object).

This PR makes the access to botCache and anonymousCache more robust.

@tdonohue tdonohue added bug 1 APPROVAL pull request only requires a single approval to merge performance / caching Related to performance, caching or embedded objects labels Aug 2, 2023
@alanorth
Copy link
Contributor

alanorth commented Aug 3, 2023

Thanks @saschaszott. I tried to test this by building Angular on DSpace 7.6 and I'm running in dev mode with both the botCache and anonymousCache enabled. I see some errors in my browser console, but no TypeError or access on undefined object. How do I know this is (or isn't) working as intended?

@saschaszott
Copy link
Contributor Author

We are using the dist Dockerfile (which uses pm2-runtime to start the server-side application). We found the TypeError in the container's log output when we try to access the home page of our DSpace instance (/).

@artlowel
Copy link
Member

server.ts is the production server. It isn't (or shouldn't be) used in development mode. Development mode uses angular's built in server (ng serve). So the problem this PR is trying to fix should never arise.

I don't use docker for dspace-angular so I can't tell you how you're supposed start the dev server using docker. Perhaps @tdonohue can shed some light on this?

@tdonohue
Copy link
Member

tdonohue commented Aug 24, 2023

@saschaszott : There are two dspace-angular Docker images available (https://hub.docker.com/r/dspace/dspace-angular)

  • dspace-7_x (or latest for main branch) runs the UI in development mode. In this scenario, SSR caching (botCache and anonymousCache) is not used. SSR is disabled in development mode.
  • dspace-7_x-dist (or latest-dist for main branch) runs the UI in production mode (using pm2). In this scenario, SSR caching can be used (based on your configuration in config.prod.yml) as SSR is enabled.

I tried spinning up latest main branch code in Docker using the latest image from dspace/dspace (backend) and the latest-dist image from dspace/dspace-angular (frontend). I cannot reproduce the error.

Here's how I started everything up locally in Docker:

  1. Started up backend (from main branch) using instructions in README
cd [backend-source]
docker-compose -f docker-compose.yml -f docker-compose-cli.yml pull
docker-compose -p d7 up -d
  1. Then, started up the frontend (from it's own main branch) using the -dist version:
cd [frontend-source]
docker-compose -f docker/docker-compose-dist.yml pull
docker-compose -p d7 -f docker/docker-compose-dist.yml up -d
  1. Verified everything worked at http://localhost:4000/ (It does)
  2. Logged into the dspace-angular image and check PM2 logs
docker exec -it dspace-angular /bin/sh
pm2 logs

In my PM2 logs, I see the (SSR) request to the homepage, but no errors appear. I tried reloading the homepage several times (still no errors appear). I tried accessing the homepage from a different browser (Firefox instead of Chrome) still no errors appear.

So, I cannot reproduce the scenario that this PR claims to fix. I'm also unsure about the fix here... as you seem to just be replacing botCacheEnabled() and anonymousCacheEnabled() with checks for cache existence. Those caches should always exist already if the *Enabled() function returns true... as they are created in the initCache() method which runs at the moment the application starts up. https://github.com/DSpace/dspace-angular/blob/main/server.ts#L319

So, I don't understand why this PR is necessary. It almost seems to imply that you have a scenario where one of the *Enabled() function returns true, but the cache itself isn't setup. That may mean you have a configuration error or something causing the cache itself to not be setup properly. OR somehow you are running server.ts while in Development Mode, which also doesn't make sense.

@tdonohue tdonohue added the cannot reproduce Unable to reproduce at this time, so the ticket either needs more information or needs closing label Aug 24, 2023
@tdonohue
Copy link
Member

tdonohue commented Sep 8, 2023

@saschaszott : As I cannot seem to reproduce this issue & others feel this isn't a good direction, I'm going to close this PR. That said, please do let us know if you find a way for this to be reliably reproduced... we can always reopen this at a later time and re-test if needed. Thanks!

@tdonohue tdonohue closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug cannot reproduce Unable to reproduce at this time, so the ticket either needs more information or needs closing performance / caching Related to performance, caching or embedded objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants