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 timelord closing. #10630

Merged
merged 1 commit into from
Apr 20, 2022
Merged

Fix timelord closing. #10630

merged 1 commit into from
Apr 20, 2022

Conversation

fchirica
Copy link
Contributor

@fchirica fchirica commented Mar 9, 2022

Attempt to fix the infinite loop Trying to stop {chain} before its initialization. if we try to close a chain that's not in self.allow_iters.

@fchirica fchirica requested a review from mariano54 March 9, 2022 19:56
Copy link
Member

@hoffmang9 hoffmang9 left a comment

Choose a reason for hiding this comment

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

lgtm

@hoffmang9
Copy link
Member

This still needs a look @mariano54

Copy link
Contributor

@mariano54 mariano54 left a comment

Choose a reason for hiding this comment

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

I don't really understand this change & this code TBH. We should make sure that when we deploy, we have some old timelords running as well.

if chain not in self.unspawned_chains:
self.unspawned_chains.append(chain)
if chain in self.chain_type_to_stream:
del self.chain_type_to_stream[chain]
del self.chain_type_to_stream[chain]
Copy link
Contributor

Choose a reason for hiding this comment

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

we are not checking inclusion anymore. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should always be included at this point - in case it's not we have no writer to close, it'll just log the error (but I don't think it ever gets to this point).

@@ -183,25 +183,22 @@ async def _handle_client(self, reader: asyncio.StreamReader, writer: asyncio.Str

async def _stop_chain(self, chain: Chain):
try:
while chain not in self.allows_iters:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing the while loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to avoid the infinite loop that happens in rare cases (written in the description).

@hoffmang9 hoffmang9 added the ready_to_merge Submitter and reviewers think this is ready label Mar 31, 2022
@wjblanke wjblanke merged commit 370ecd4 into main Apr 20, 2022
@wjblanke wjblanke deleted the fc.timelord-fix branch April 20, 2022 18:10
emlowe pushed a commit that referenced this pull request Apr 21, 2022
justinengland pushed a commit that referenced this pull request Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants