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 Procedure level queue #1946

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

KLarpen
Copy link
Contributor

@KLarpen KLarpen commented Dec 17, 2023

Closes: #1945

API endpoint local queue params not working at all. I had investigate that the problem in Procedure class and originated from commit "Improve Procedure class PR-URL: #1569 " 434e6c1 Where was applied changes into constructor:

  • Object.assign(this, exp); removed
  • expected property concurrency of the API endpoint was replaced with queue object containing queue.concurrency
  • BUT this.concurrency not assigned directly anymore (as for example done for this.parameters.

However methods enter and leave still checkingthis.concurrency for existance as of today.

impress/lib/procedure.js

Lines 42 to 57 in 4cabe81

async enter() {
await this.application.semaphore.enter();
if (this.concurrency) {
try {
await this.semaphore.enter();
} catch (error) {
this.application.semaphore.leave();
throw error;
}
}
}
leave() {
this.application.semaphore.leave();
if (this.concurrency) this.semaphore.leave();
}

As a result local Semaphore of the Procedure never applied. There should be check for the property this.semaphore instead. It suitable for this purpose because it will be null in case API endpoint doesn't provide queue settings.

Before applying the fix I also added test case lib/procedure queue in /test/procedure.js . By the way I had fixed application stub in Procedure tests to make it comply with updated contract (was changed by #1898).

  • tests and linter show no problems (npm t)
  • tests are added/updated for bug fixes and new features
  • code is properly formatted (npm run fmt)
  • description of changes is added in CHANGELOG.md
  • update .d.ts typings

@KLarpen
Copy link
Contributor Author

KLarpen commented Dec 18, 2023

Please review @tshemsedinov . This fix didn't change contracts and looks like a good candidate for next patch release of Impress.

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

LGTM

@tshemsedinov tshemsedinov merged commit fa4d0bb into metarhia:master Dec 19, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local API endpoint queue settings doesn't work
2 participants