Skip to content

SG-35712 Prevent flaky disconnection when uploading thumbnails on publish #368

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

Merged
merged 4 commits into from
Mar 11, 2025

Conversation

carlos-villavicencio-adsk
Copy link
Contributor

@carlos-villavicencio-adsk carlos-villavicencio-adsk commented Feb 11, 2025

There's a flaky disconnection when the publisher uploads the thumbnail. The most common errors are:

  • Connection closed by peer
  • URLopen error EOF occurred in violation of protocol ssl.c:1006

They seem to be related to TLS handshakes. While testing on local machines we consistently reproduced this with the following change: Move the connection opener to the retry block and change CACertsHTTPSHandler parent class.

@@ -4330,13 +4330,13 @@ def connect(self):
)


class CACertsHTTPSHandler(urllib.request.HTTPSHandler):
class CACertsHTTPSHandler(urllib.request.HTTPHandler):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a clear answer for this. I was a trial/error change that made sense to me according to keep consistency on naming. Also, we're using SSL with TLS.


attempt = 1
while attempt <= self.MAX_ATTEMPTS:
# Perform the request
try:
opener = self._build_opener(FormPostHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

Side question: why don't we use httplib2 for those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to answer this. This line was moved from outside the retry block, so in case it fails, the retry can start a new connection. I don't know the details of the original design.

@carlos-villavicencio-adsk carlos-villavicencio-adsk merged commit 666fa0c into master Mar 11, 2025
24 checks passed
@carlos-villavicencio-adsk carlos-villavicencio-adsk deleted the ticket/SG-35712 branch March 11, 2025 21:02
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