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

Update SessionTracker to use BackgroundTaskService #1184

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Mar 10, 2021

Goal

Updates SessionTracker to use BackgroundTaskService rather than Async. This allows for better control over asynchronous operations and negates the need for a Semaphore which was a source of non-deterministic behaviour that could lead to some session requests not immediately being sent. This is based on changes made in #1182.

Changeset

  • Create a BackgroundTaskService in the Client
  • Updated SessionTracker to require a BackgroundTaskService constructor param
  • Refactored SessionTracker to submit two types of task to the session request executor: flushing stored sessions from disk, and flushing in-memory sessions

Testing

Added a unit test to confirm that requests on the session tracker are confined to a thread, otherwise relied on existing E2E test coverage for sessions.

@fractalwrench fractalwrench marked this pull request as ready for review March 10, 2021 16:45
Base automatically changed from background-task-service to integration/road-1175-identify-crashes-on-launch March 11, 2021 14:42
Copy link
Contributor

@twometresteve twometresteve left a comment

Choose a reason for hiding this comment

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

LGTM

@fractalwrench fractalwrench merged commit b697813 into integration/road-1175-identify-crashes-on-launch Mar 11, 2021
@fractalwrench fractalwrench deleted the session-tracker-bg-changes branch March 11, 2021 15: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