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

[Components] In some rare cases the method component.stop() can work incorrectly #6358

Closed
drew2a opened this issue Sep 24, 2021 · 2 comments · Fixed by #6419
Closed

[Components] In some rare cases the method component.stop() can work incorrectly #6358

drew2a opened this issue Sep 24, 2021 · 2 comments · Fixed by #6419
Assignees

Comments

@drew2a
Copy link
Contributor

drew2a commented Sep 24, 2021

During the refactoring (#6335), we found the following potential problem:

In some rare cases the method component.stop() may not work correctly.

The current implementation (simplified) is the following:

class Session:
    async def start(self, failfast=True):
        coros = [comp.start() for comp in self.components.values()]
        await gather(*coros, return_exceptions=not failfast)

    async def shutdown(self):
        await gather(*[create_task(component.stop()) for component in self.components.values()])

That means whatever comp.start() was failed or succeeded, the corresponding component.stop() will be called during a shutdown.

That means component.stop() should distinguish initialized and uninitialized fields of the corresponding class (this is not currently being done).

@drew2a drew2a added this to the Backlog milestone Sep 24, 2021
@Solomon1732
Copy link
Contributor

What is the way to check for a successfully started component? I'm guessing comp.started.is_set()?

kozlovsky added a commit to kozlovsky/tribler that referenced this issue Oct 6, 2021
kozlovsky added a commit to kozlovsky/tribler that referenced this issue Oct 6, 2021
kozlovsky added a commit to kozlovsky/tribler that referenced this issue Oct 6, 2021
kozlovsky added a commit to kozlovsky/tribler that referenced this issue Oct 6, 2021
kozlovsky added a commit to kozlovsky/tribler that referenced this issue Oct 6, 2021
kozlovsky added a commit to kozlovsky/tribler that referenced this issue Oct 6, 2021
kozlovsky added a commit that referenced this issue Oct 6, 2021
@kozlovsky
Copy link
Contributor

kozlovsky commented Oct 6, 2021

@Solomon1732 yes, but in 2730ab8 it was renamed to comp.started_event.is_set() to avoid possible mistake with a Boolean flag:

if comp.started:   # will evaluate to True even if component has not started yet 
    ...

Now it should look clearer:

if comp.started_event.is_set():
    ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants