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 components startup #6507

Merged
merged 8 commits into from
Oct 29, 2021
Merged

Conversation

kozlovsky
Copy link
Contributor

@kozlovsky kozlovsky commented Oct 27, 2021

This PR fixes the following problem: it becomes impossible to close Tribler when a component's run method raises an error.

The reason for this behavior is the following:

  1. Session was started with the default failfast=True option;
  2. Its start method propagates the first exception from a failed component.run method;
  3. Tribler core event loop exits before Tribler GUI establish a connection with Core;
  4. Tribler GUI tries to connect with Core and waits indefinitely, ignoring attempts to close it.

The desired behavior is the following:

  1. If some component crashes on start, Tribler should run anyway because the remaining functionality with a failed component may still satisfy the user's needs.
  2. The error raised during the component's start should be passed to the GUI to be reported in the usual way to Sentry if the user agrees.
  3. One component's fail can cause cascade failures of other components. These failures should not be reported as they are secondary.

This PR does the following to fix the problem:

  1. It starts Session with the failfast=False option.
  2. When an exception is raised by a component's run method, it is raised again in a separate branch to be processed by Sentry (raising it in the original branch will stop the event loop).
  3. New should_stop field was added to the exception's notification from Core to GUI to prevent automatic stopping of Tribler after the error was reported to Sentry.

As an example of error, the case of TagsDatabase failure was considered caused by the incorrect structure of tables. I added defensive checks to other parts of the code in a separate commit so Tribler could successfully start even if TagsComponent could not run.

As a separate commit, I added a possibility to have errors reported from Core to GUI, which are not critical enough to cause Tribler shutdown, so they are passed with the should_stop=False flag. In case of repeating errors, new occurrences will be suppressed silently not to disturb the user interface.

Also, I noticed that errors in REST API are not reported to Sentry and displayed to the user in a separate dialog window. It is too big a task to be included in this PR, so I added a TODO option and will open a related issue.

Related to #6263

@kozlovsky kozlovsky force-pushed the fix/components_startup branch from 5d54456 to a574f53 Compare October 27, 2021 16:45
@kozlovsky kozlovsky marked this pull request as ready for review October 28, 2021 06:57
@kozlovsky kozlovsky requested review from a team and drew2a and removed request for a team October 28, 2021 06:57
Copy link
Contributor

@drew2a drew2a left a comment

Choose a reason for hiding this comment

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

Great work!

I wrote some comments about the PR and Pylint and Flake8 should be satisfied before PR can be merged.

@kozlovsky kozlovsky force-pushed the fix/components_startup branch 5 times, most recently from b3823be to d622a2c Compare October 28, 2021 13:51
@kozlovsky kozlovsky requested a review from drew2a October 28, 2021 13:56
@kozlovsky
Copy link
Contributor Author

I changed the logic for the component to have tribler_should_stop_on_component_error flag which is True by default. If it is set to False, Tribler will continue working even if the component crashed with an error.

drew2a
drew2a previously approved these changes Oct 28, 2021
drew2a
drew2a previously approved these changes Oct 28, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@kozlovsky kozlovsky merged commit c834092 into Tribler:main Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants