Skip to content

Conversation

PProfizi
Copy link
Contributor

When debugging with PyCharm on my local machine, I kept having a lot of tests fail due to a TimeoutError during Server start-up. It turned out the default timeout of 10s was too little, so I added a second attempt with a timeout of 20s. If it still fails, then it raises the TimeoutError. This did the trick for me.

@PProfizi PProfizi requested a review from cbellot000 March 17, 2022 17:49
@PProfizi PProfizi enabled auto-merge (squash) March 17, 2022 18:15
except TimeoutError:
if timed_out:
break
timeout *= 2.
Copy link
Contributor

Choose a reason for hiding this comment

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

After a just a few attempts this timeout would grow quite large. Perhaps consider:

Suggested change
timeout *= 2.
timeout += 10.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akaszynski in the current implementation, the time out is only increased once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it fails for something other than a TimeoutError, it does not go through timeout *= 2, and it can only go through it once. The thing is I did not want to make an assumption on the absolute timeout value given in input (once timeout comes from the calling function) so doubling made more sense than adding ten something.

@PProfizi PProfizi merged commit 5e92572 into master Mar 18, 2022
@PProfizi PProfizi deleted the fix/Timeout_Server_2tries branch March 18, 2022 07:47
@cbellot000
Copy link
Contributor

@PProfizi you also told me that the timeout parameter cannot be changed up to the last method using it. Could you please update that as well?

@cbellot000
Copy link
Contributor

@PProfizi did you check that when the start_server function is rerun, the first started server (the one we failed ton connect to) is stopped and doesn't leave an unwanted process opened?

@PProfizi PProfizi restored the fix/Timeout_Server_2tries branch March 18, 2022 08:15
@PProfizi PProfizi deleted the fix/Timeout_Server_2tries branch March 18, 2022 08:15
@PProfizi
Copy link
Contributor Author

I'll deal with @cbellot000 's last comments in a new PR. Sorry, I put this one on automatic merge.

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