Fixed slow startup problems

[tjcouch-sil]

Resolves #1176 (hopefully)

• Made contribution-based services wait for extensions to load before giving out data where reasonably straight-forward to do (menu service especially didn't really get this benefit because it wasn't as simple as doing the others. But it is a data provider, so hopefully consumers will listen to updates and get the updated data anyway)
  • Fixes dotnet process getting settings before extensions load
• Added retry to getting project metadata if not found in process first 30 seconds
  • Fixes not seeing any projects when clicking menu item "Open Scripture Editor" before the dotnet process finishes loading

---

This change is 

[lyonsil]

Looks good overall. I have a few thoughts to consider.

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)

---

src/extension-host/services/contribution.service.ts line 208 at r1 (raw file):

  // Do things in response to finishing resyncing contributions
  await Promise.all([...onDidResyncContributionsSubscriptions].map(async (callback) => callback()));

Do you think the callbacks should be wrapped in try/catch?

---

src/shared/services/project-lookup.service.test.ts line 40 at r1 (raw file):

beforeAll(() => {
  jest.useFakeTimers({

👀

I learn something new every day.

---

src/shared/models/project-lookup.service-model.ts line 347 at r1 (raw file):

 * process started
 */
const GRACE_PERIOD_WAIT_TIME_MS = 5 * 1000;

This seems a little slower than we need to be. I would think retrying every second (or a little sooner) would help be more responsive for people than every 5 seconds.

---

src/shared/models/project-lookup.service-model.ts line 482 at r1 (raw file):

    // Intentionally stopping this method execution to try getting project metadata again
    // eslint-disable-next-line no-await-in-loop
    allProjectsMetadataArray = await internalGetMetadata(options);

Making this function recursive and extending an already long function to be even longer makes me a little uneasy. It probably works OK for now, but there could be some weird maintenance impacts (e.g., if someone needs to extend the grace time to a few minutes for really slow machines and shrink the wait time to multiple times per second, potentially resulting in stack overflows on slow machines). I think it would be a little cleaner to put this code into a new function (e.g., internalGetMetadataWithRetries) and change all the current callers of internalGetMetadata to point to the new function.

[tjcouch-sil]

Reviewable status: 10 of 12 files reviewed, all discussions resolved (waiting on @lyonsil)

---

src/extension-host/services/contribution.service.ts line 208 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Do you think the callbacks should be wrapped in try/catch?

Good idea! Done.

---

src/shared/models/project-lookup.service-model.ts line 347 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This seems a little slower than we need to be. I would think retrying every second (or a little sooner) would help be more responsive for people than every 5 seconds.

We can try one second. I thought since this gets called a lot and will continually have problems until things start up, might as well wait a while before trying again since chances are it will be a while before things start up well. But this is fine, too, so why not :)

---

src/shared/models/project-lookup.service-model.ts line 482 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Making this function recursive and extending an already long function to be even longer makes me a little uneasy. It probably works OK for now, but there could be some weird maintenance impacts (e.g., if someone needs to extend the grace time to a few minutes for really slow machines and shrink the wait time to multiple times per second, potentially resulting in stack overflows on slow machines). I think it would be a little cleaner to put this code into a new function (e.g., internalGetMetadataWithRetries) and change all the current callers of internalGetMetadata to point to the new function.

Are you tryna tell me I'm not allowed to achieve a programmer's highest honor and finally cause a StackOverflow.com in the build???????????????

Done :)

---

src/shared/services/project-lookup.service.test.ts line 40 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

👀

I learn something new every day.

Me too haha I had to look this up

[lyonsil]

Thanks for the updates.

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved