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 #295 and #480, Resolve app table scanning race conditions #598

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Apr 9, 2020

Describe the contribution

Fixes #295
Fixes #480
Partially addresses #567 - runloop now increments task counter instead of main app execution counter

Create a new background job to handle the maintenance tasks that had been performed in the ES main task as part of the CFE_ES_ScanAppTable() routine.

All app state changes, including those invoked by messages, are now handled by this job.

This also slightly changes the semantics of CFE_ES_RunLoop and CFE_ES_ExitApp. Now, the CFE_ES_RunLoop routine no longer requires a RunStatus buffer. Instead, the only thing that matters is the RunStatus value that is eventually passed to CFE_ES_ExitApp after the shutdown is complete. This should be mostly backward compatible, as the recommended app pattern would pass the same value to both functions.

This commit also fixes #480, as the value passed to CFE_ES_ExitApp will not override a request that was already pending.

Testing performed
Build with ENABLE_UNIT_TESTS=TRUE for native (x86-64 Linux) and RTEMS (i686-rtems4.11)

  • Run unit tests and confirm all passing
  • Check coverage report (lcov) and confirm no reduction in coverage for modified functions, and newly added functions are fully covered.
  • Run CFE and send various restart/reload/delete requests for SAMPLE_APP
  • Confirm that SAMPLE_APP exits itself and is restarted/reloaded/deleted correctly by ES.
  • Confirm that if SAMPLE_APP does not exit itself, it is still forcibly deleted by ES
  • Confirm same behavior on RTEMS

Expected behavior changes
No impact to CMD/TLM interface or API. Internal changes only.

System(s) tested on

  • Ubuntu 18.04 LTS 64 bit (native)
  • QEMU (i686-rtems4.11)

Additional context
This reuses the background task concept introduced in pull #595. Will rebase this changeset as soon as that pull request makes it into a stable baseline.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems Inc.

Create a new background job to handle the maintenance tasks
that had been performed in the ES main task as part of the
CFE_ES_ScanAppTable() routine.

All app state changes, including those invoked by messages,
are now handled by this job.

This also slightly changes the semantics of CFE_ES_RunLoop and
CFE_ES_ExitApp.  Now, the CFE_ES_RunLoop routine no longer requires
a RunStatus buffer.  Instead, the only thing that matters is the
RunStatus value that is eventually passed to CFE_ES_ExitApp after
the shutdown is complete.  This should be mostly backward compatible,
as the recommended app pattern would pass the same value to
both functions.

This commit also fixes nasa#480, as the value passed to CFE_ES_ExitApp
will not override a request that was already pending.
@jphickey
Copy link
Contributor Author

Force-push: rebased to latest master branch

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 10, 2020
@jphickey jphickey requested review from skliper and a user April 10, 2020 02:08
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good. I like the background job design.

@astrogeco
Copy link
Contributor

CCB 20200415 - APPROVED

@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB CCB - 20200415 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 20, 2020
@astrogeco astrogeco changed the base branch from master to integration-candidate April 21, 2020 22:17
@astrogeco astrogeco merged commit a4a48bd into nasa:integration-candidate Apr 21, 2020
@astrogeco astrogeco changed the title Fix #295, Resolve app table scanning race conditions Fix #295 and #480, Resolve app table scanning race conditions Apr 27, 2020
@jphickey jphickey deleted the fix-295-apptable-scan branch May 1, 2020 17:32
@skliper skliper added this to the 6.8.0 milestone Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
3 participants