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.

Sign up for GitHub

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

Jump to bottom

[Endpoint] Fix saga to ensure they are started only after store is created #55245

Merged
merged 3 commits into from
Jan 21, 2020
Merged

[Endpoint] Fix saga to ensure they are started only after store is created #55245

merged 3 commits into from
Jan 21, 2020

Conversation

paul-tavares
Copy link
Contributor

Summary

Fixed the Saga Middleware creator factory to ensure that saga is not run (started) until the store has been created (otherwise it could cause store actions to be missed). Also implemented a mechanism for "stopping" the saga when the Endpoint app is unmounted from DOM (which could occur often under the New Platform)

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@paul-tavares
Fix saga to ensure they are started only after store is created
585508e 
- added stop()/start() methods to the Saga Middleware creator factory
- adjust tests based on changes
- changed application `renderApp` to stop sagas when react app is unmounted
@paul-tavares paul-tavares added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 Team:Endpoint Response Endpoint Response Team Team:Endpoint Data Visibility Team managing the endpoint resolver Team:Endpoint Management labels Jan 17, 2020
peluja1012
peluja1012 reviewed Jan 21, 2020
View reviewed changes
x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.ts Outdated
middleware.stop = () => {
running = false;

// In order to actually stop the iterator instances, we need to force the loop to run one last time
Copy link
Contributor

Choose a reason for hiding this comment

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

After setting running to false, the following actions won't have any effect, right? So why do we need to enqueue another action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @peluja1012 - correct, this action will have no effect other than to force the loop to exit. The generator loop is paused awaiting the resolution of the current pending Promise. So when this action here is queued, that promise will resolve and the loop will exit out..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peluja1012 - Rob had suggested perhaps using a second Promise to control the "stop action" and use Promise.race inside of the while (running) loop. Its a another approach and if that seems clearer to all, I can implement it that way.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@paul-tavares paul-tavares merged commit 5a5bade into elastic:master Jan 21, 2020
@paul-tavares paul-tavares deleted the task/stop-saga-on-app-unmount branch January 21, 2020 19:21
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 22, 2020
@gmmorris
Merge branch 'master' into pr55026
0dae5de 
* master: (38 commits)
  [ML] Fix counters and percentages for array fields on the Data visualizer page (elastic#55209)
  [SIEM][Detection Engine] Tags being turned into null
  rules part deux (elastic#55507)
  [DOCS] Add tip for using elasticsearch-certutil http command (elastic#55357)
  [SIEM][Detection Engine] Critical blocker, fixes schema accepting values it should not (elastic#55488)
  [SIEM] Detections create prepackage rules (elastic#55403)
  [Reporting] Convert CSV Export libs to Typescript (elastic#55117)
  [Maps] show field type icons in data driven styling field select (elastic#55166)
  Adds event log for actions and alerting (elastic#45081)
  [SIEM][Detection Engine] Fixes critical blocker where signals on signals are not operating
  [SIEM][Detection Engine] Critical blocker, adds need REST prefix for cloud
  remove incorrect config (elastic#55427)
  Retain pinned filters when loading and clearing saved queries (elastic#54307)
  Resolver zoom, pan, and center controls (elastic#55221)
  Skip failing endpoint saga tests
  [skip-ci] Update migration guide to add rendering service example (elastic#54744)
  [DOCS] Updates to heat map page (elastic#55097)
  [Endpoint] Fix saga to start only after store is created and stopped on app unmount (elastic#55245)
  [Logs UI] Use the correct icons and labels in the feature cont… (elastic#55292)
  [Uptime] Handle locations with names but no geo data (elastic#55234)
  ...
@kevinlog kevinlog added the Feature:Endpoint Elastic Endpoint feature label Feb 18, 2020
oatkiller pushed a commit that referenced this pull request Feb 18, 2020
@kevinlog @elasticmachine
@legrego @nnamdifrankie @dplumlee @paul-tavares @peluja1012 @pzl @madirey @charlie-pichette @parkiino @pgayvallet @alexwizp
[7.x] [Endpoint] Adding Endpoint to 7.x (#57867)
1cfb08c 
* Add Endpoint plugin and Resolver embeddable (#51994)

* Add functional tests for plugins to x-pack (so we can do a functional test of the Resolver embeddable)
* Add Endpoint plugin
* Add Resolver embeddable
* Test that Resolver embeddable can be rendered

 Conflicts:
	x-pack/.i18nrc.json
	x-pack/test/api_integration/apis/index.js

* [Endpoint] Register endpoint app (#53527)

* register app, create functional test

* formatting

* update tests

* adjust test data for endpoint

* add endpoint tests for testing spaces, app enabled, disabled, etc

* linting

* add read privileges to endpoint

* rename variable since its used now

* remove deprecated context

* remove unused variable

* fix type check

* correct test suite message

Co-Authored-By: Larry Gregory <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Larry Gregory <[email protected]>

* [Endpoint] add react router to endpoint app (#53808)

* add react router to endpoint app

* linting

* linting

* linting

* correct tests

* change history from hash to browser, add new test util

* remove default values in helper functions

* fix type check, use FunctionComponent as oppsed to FC

* use BrowserRouter component

* use BrowserRouter component lin

* add comments to test framework, change function name to include browserHistory

Co-authored-by: Elastic Machine <[email protected]>

* EMT-issue-65: add endpoint list api (#53861)

add endpoint list api

* EMT-65:always return accurate endpoint count (#54423)

EMT-65:always return accurate endpoint count, independent of paging properties

* Resolver component w/ sample data (#53619)

Resolver is a map. It shows processes that ran on a computer. The processes are drawn as nodes and lines connect processes with their parents.

Resolver is not yet implemented in Kibana. This PR adds a 'map' type UX. The user can click and drag to pan the map and zoom using trackpad pinching (or ctrl and mousewheel.)

There is no code providing actual data. Sample data is included. The sample data is used to draw a map. The fundamental info needed is:

process names
the parent of a process
With this info we can topologically lay out the processes. The sample data isn't yet in a realistic format. We'll be fixing that soon.

Related issue: elastic/endpoint-app-team#30

* Resolver test plugin not using mount context. (#54933)

Mount context was deprecated. Use core.getStartServices() instead.

* Resolver nonlinear zoom (#54936)

* [Endpoint] add Redux saga Middleware and app Store (#53906)

* Added saga library
* Initialize endpoint app redux store

* Resolver is overflow: hidden to prevent obscured elements from showing up (#55076)

* [Endpoint] Fix saga to start only after store is created and stopped on app unmount (#55245)

- added `stop()`/`start()` methods to the Saga Middleware creator factory
- adjust tests based on changes
- changed application `renderApp` to stop sagas when react app is unmounted

* Resolver zoom, pan, and center controls (#55221)

* Resolver zoom, pan, and center controls

* add tests, fix north panning

* fix type issue

* update west and east panning to behave like google maps

* [Endpoint] FIX: Increase tests `sleep` default duration back to 100ms (#55492)

Revert `sleep()` default duration, in the saga tests, back to 100ms in order to prevent intermittent failures during CI runs.

Fixes #55464
Fixes #55465

* [Endpoint] EMT-65: make endpoint data types common, restructure (#54772)

[Endpoint] EMT-65: make endpoint data types common, use schema changes

* Basic Functionality Alert List (#55800)

* sets up initial grid and data type

* data feeds in from backend but doesnt update

* sample data feeding in correctly

* Fix combineReducers issue by importing Redux type from 'redux' package

* Add usePageId hook that fires action when user navigates to page

* Strict typing for middleware

* addresses comments and uses better types

* move types to common/types.ts

* Move types to endpoint/types.ts, address PR comments

blah 2

Co-authored-by: Pedro Jaramillo <[email protected]>

* [Endpoint] Add Endpoint Details route (#55746)

* Add Endpoint Details route

* add Endpoint Details tests

* sacrifices to the Type gods

* update to latest endpoint schema

Co-authored-by: Elastic Machine <[email protected]>

* [Endpoint] EMT-67: add kql support for endpoint list (#56328)

[Endpoint] EMT-67: add kql support for endpoint list

* [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (#56538)

* ERT-82 ERT-83 ERT-84 (partial): Add Alert List API with pagination

* Better type safety for alert list API

* Add Test to Verify Endpoint App Landing Page (#57129)

 Conflicts:
	x-pack/test/functional/page_objects/index.ts

* fixes render bug in alert list (#57152)

Co-authored-by: Elastic Machine <[email protected]>

* Resolver: Animate camera, add sidebar (#55590)

This PR adds a sidebar navigation. clicking the icons in the nav will focus the camera on the different nodes. There is an animation effect when the camera moves.

 Conflicts:
	yarn.lock

* [Endpoint] Task/basic endpoint list (#55623)

* Adds host management list to endpoint security plugin

Co-authored-by: Elastic Machine <[email protected]>

* [Endpoint] Policy List UI route and initial view (#56918)

* Initial Policy List view

* Add `endpoint/policy` route and displays Policy List
* test cases (both unit and functional)

Does not yet interact with API (Ingest).

* Add ApplicationService app status management (#50223)

This was already backported, but changes to endpoint app could not be
backported, since endpoint app itself hadn't been backported. Now that
the endpoint app is backported, reapply the endpoint specific changes
from the original commit.

* Implements `getStartServices` on server-side (#55156)

This was already backported, but changes to endpoint app could not be
backported, since endpoint app itself hadn't been backported. Now that
the endpoint app is backported, reapply the endpoint specific changes
from the original commit.

* [ui/utils/query_string]: Remove unused methods & migrate apps to querystring lib (#56957)

This was already backported, but changes to endpoint app could not be
backported, since endpoint app itself hadn't been backported. Now that
the endpoint app is backported, reapply the endpoint specific changes
from the original commit.

Co-authored-by: Kevin Logan <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: nnamdifrankie <[email protected]>
Co-authored-by: Davis Plumlee <[email protected]>
Co-authored-by: Paul Tavares <[email protected]>
Co-authored-by: Pedro Jaramillo <[email protected]>
Co-authored-by: Dan Panzarella <[email protected]>
Co-authored-by: Madison Caldwell <[email protected]>
Co-authored-by: Charlie Pichette <[email protected]>
Co-authored-by: Candace Park <[email protected]>
Co-authored-by: Pierre Gayvallet <[email protected]>
Co-authored-by: Alexey Antonov <[email protected]>
@tsullivan tsullivan mentioned this pull request May 21, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peluja1012 peluja1012 peluja1012 approved these changes

@kevinlog kevinlog Awaiting requested review from kevinlog

@oatkiller oatkiller Awaiting requested review from oatkiller

@dplumlee dplumlee Awaiting requested review from dplumlee

@bkimmel bkimmel Awaiting requested review from bkimmel

@kqualters-elastic kqualters-elastic Awaiting requested review from kqualters-elastic

@parkiino parkiino Awaiting requested review from parkiino

Assignees
No one assigned
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver Team:Endpoint Response Endpoint Response Team v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paul-tavares @kibanamachine @peluja1012 @kevinlog