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

Fix error in match & allocate service crashing app #869

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

Cruikshanks
Copy link
Member

https://eaflood.atlassian.net/browse/WATER-4375

Spotted whilst working on Fix licence issues failing to insert for review. When PersistAllocatedLicenceToResultsService errored trying to insert a record it was causing an uncaught exception that was crashing the app.

After investigating the issue we have spotted it is because MatchAndAllocateService is using a forEach() to call asynchronous code.

If we switch to a for...of loop instead any errors thrown within the loop, even from PersistAllocatedLicenceToResultsService get caught as expected by the top level try/catch in ProcessBillRun and the bill run marked as errored.

Also, whilst looking into this we spotted that DetermineLicenceIssuesService has been made async even though it is doing nothing asynchronous. We use this opportunity to fix that as well.

https://eaflood.atlassian.net/browse/WATER-4375

Spotted whilst working on [Fix licence issues failing to insert for review](#867). When `PersistAllocatedLicenceToResultsService` errored trying to insert a record it was causing an uncaught exception that was crashing the app.

After investigating the issue we have spotted it is because `MatchAndAllocateService` is using a `forEach()` to call asynchronous code.

- [Using async/await in a forEach loop (you can't)](https://blog.devgenius.io/using-async-await-in-a-foreach-loop-you-cant-c174b31999bd)
- [Do not use forEach with async-await](https://gist.github.com/joeytwiddle/37d2085425c049629b80956d3c618971)

If we switch to a `for...of` loop instead any errors thrown within the loop, even from `PersistAllocatedLicenceToResultsService` get caught as expected by the top level `try/catch` in `ProcessBillRun` and the bill run marked as errored.

Also, whilst looking into this we spotted that `DetermineLicenceIssuesService` as been made `async` even though it is doing nothing asynchronous. We use this opportunity to fix that as well.
@Cruikshanks Cruikshanks added the bug Something isn't working label Mar 27, 2024
@Cruikshanks Cruikshanks self-assigned this Mar 27, 2024
@Cruikshanks Cruikshanks marked this pull request as ready for review March 27, 2024 22:45
Copy link
Contributor

@Beckyrose200 Beckyrose200 left a comment

Choose a reason for hiding this comment

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

@Cruikshanks Cruikshanks merged commit b0bae7b into main Mar 28, 2024
6 checks passed
@Cruikshanks Cruikshanks deleted the error-in-match-and-allocate-crashes-app branch March 28, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants