Skip to content

fix (event processor) [OASIS-5907]: stop promise tracks in-flight dispatcher requests #397

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

Merged
merged 12 commits into from
Feb 18, 2020

Conversation

mjc1283
Copy link
Contributor

@mjc1283 mjc1283 commented Feb 18, 2020

Summary

Add tracking functionality to EventProcessor for every request sent by its dispatcher. This is implemented in a RequestTracker class.

RequestTracker exposes a method (onRequestsComplete) to get a Promise that fulfills after the completion of requests in-flight at the time of the call.

stop is updated to return the promise from RequestTracker onRequestsComplete.

Test plan

  • New and existing unit tests
  • Manually tested in browser, Node, & Lambda environments

Issues

https://optimizely.atlassian.net/browse/OASIS-5907

@mjc1283 mjc1283 added the WIP label Feb 18, 2020
@mjc1283 mjc1283 self-assigned this Feb 18, 2020
@coveralls
Copy link

coveralls commented Feb 18, 2020

Coverage Status

Coverage remained the same at 97.559% when pulling 79cca5f on mcarroll/OASIS-5907-simple into eb47310 on master.

@mjc1283 mjc1283 removed the WIP label Feb 18, 2020
@mjc1283 mjc1283 marked this pull request as ready for review February 18, 2020 16:25
@mjc1283 mjc1283 requested a review from a team as a code owner February 18, 2020 16:25
@mjc1283 mjc1283 removed their assignment Feb 18, 2020
* limitations under the License.
*/

class RequestTracker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: please include a doc comment to describe this class

try {
// swallow, an error stopping this queue should prevent this from stopping
return this.queue.stop()
this.queue.stop()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be awaiting this also? Does it matter if onRequestsComplete resolves before this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's redundant. The Promise returned from queue.stop is based on the request it triggers at that time, but all requests are also tracked by the RequestTracker.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

Lgtm

@mjc1283 mjc1283 merged commit d582ffa into master Feb 18, 2020
@mjc1283 mjc1283 deleted the mcarroll/OASIS-5907-simple branch February 18, 2020 19:18
@clementallen
Copy link

clementallen commented Feb 18, 2020

Hi! Will this resolve #382?

@mjc1283
Copy link
Contributor Author

mjc1283 commented Feb 26, 2020

Hi @clementallen,

Yes, we released v3.5.0 with this change and it should solve the issue. Thanks for the suggestion. If you could try it out and let us know how it's working for you, we'd appreciate it.

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.

4 participants