Skip to content

fix(event processor): Stop accepting events after stop is called #355

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 2 commits into from
Aug 29, 2019

Conversation

mjc1283
Copy link
Contributor

@mjc1283 mjc1283 commented Aug 27, 2019

Summary

Before this change, event processor would continue processing user events after its stop method was called. If the batch size limit was reached after stop was called, it would dispatch a log event as normal. Worse, the internal queue wasn't being emptied when stop was called, which could lead to the same user event being included in two different log events.

With this change, the event processor's internal queue stops accepting events after the event processor is stopped. A warning message is logged, but the event is dropped.

Test plan

Updated unit tests. Will manually test upon integration of a new version in optimizely-sdk.

@mjc1283 mjc1283 requested a review from a team as a code owner August 27, 2019 23:19
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
## [Unreleased]
Changes that have landed but are not yet released.

### Changed
- Introduce an explict`DefaultEventQueue` no longer enqueues additional events after being stopped
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will correct this, and move to "Fixed" section

@mjc1283 mjc1283 removed their assignment Aug 27, 2019
@coveralls
Copy link

coveralls commented Aug 27, 2019

Coverage Status

Coverage remained the same at 97.598% when pulling 8eea114 on mcarroll/fix-event-processor-after-stop into f71646a on master.

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Changes look good. Great catch.

@mjc1283 mjc1283 merged commit 8fc6d38 into master Aug 29, 2019
@mjc1283 mjc1283 deleted the mcarroll/fix-event-processor-after-stop branch August 29, 2019 17:08
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.

3 participants