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

Retry all HttpException & SocketException in to methods (workaround http -1) #373

Closed
wants to merge 4 commits into from

Conversation

borgstrom
Copy link

@borgstrom borgstrom commented Aug 23, 2017

This PR is the result of needing to work around https://issues.jenkins-ci.org/browse/JENKINS-45142

We were seeing the following error quite frequently when scanning all repos in our github org:

ERROR: Server returned HTTP response code: -1, message: 'null' for URL: https://api.github.com
java.net.SocketException: Socket closed

The retry code in parse was not working as expected, so this moves the retry code directly into the to methods.

Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Approach sounds fine to me, especially if it has been demonstrated to work better than the original fix. Awaiting tuning of status code handling.

while (true) {
try {
return __to(tailApiUrl, type, instance);
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be limited to SocketTimeoutException, as in the original?

Copy link
Author

Choose a reason for hiding this comment

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

No, because HttpException extends from IOException.

I will limit the retry mechanism to only HttpExceptions with status codes >= 400 && SocketException-- I feel that there's merit to retrying other socket exceptions, such as connection reset errors.

@jglick
Copy link
Contributor

jglick commented Aug 23, 2017

This amends #359 FTR.

@borgstrom borgstrom changed the title Retry all IOException in to methods (workaround http -1) Retry all HttpException & SocketException in to methods (workaround http -1) Aug 23, 2017
@borgstrom
Copy link
Author

I have just uploaded a snapshot of github-api-plugin with this branch to my jenkins instance and kicked off a full org scan.

@borgstrom
Copy link
Author

This implementation is not working as expected. We're still seeing -1 status HTTP responses in our logs periodically:

[Tue Aug 29 10:44:46 PDT 2017] Push event to branch github-reading in repository NerdWallet/museum UPDATED event from 192.30.252.34 ⇒ https://jenkins.nerdwallet.io/github-webhook/ with timestamp Tue Aug 29 10:44:40 PDT 2017 processed in 0.44 sec
    Checking branch github-reading

  Getting remote pull request #39...

  1 branches were processed

  Checking pull-requests...

    Checking pull request #39
      ‘Jenkinsfile’ found
ERROR: Server returned HTTP response code: -1, message: 'null' for URL: https://api.github.com/repos/NerdWallet/museum/pulls/39
java.net.SocketException: Socket closed

I will spend some time on this today or tomorrow.

@jglick
Copy link
Contributor

jglick commented Sep 8, 2017

@borgstrom any progress?

@ErikDahlinghaus
Copy link

This is really causing problems at my organization. Any updates on this issue? Thanks.

@bitwiseman
Copy link
Member

@borgstrom Progress?
@ErikDahlinghaus Are you still seeing the issue? Perhaps this is a non-repro at this point.

@borgstrom
Copy link
Author

We have not had any problems in a long time. I consider this abandoned at this point.

@borgstrom borgstrom closed this May 17, 2019
@ghost
Copy link

ghost commented Jun 25, 2019

Could this pull request be reopened? The current retry mechanism does not have a delay and it fails when there are intermittent network issues.

@bitwiseman bitwiseman reopened this Jun 25, 2019
@bitwiseman bitwiseman closed this Jun 25, 2019
@bitwiseman
Copy link
Member

@bogdansuta-intel
It seems the original PR owner isn't seeing this issue any more. I encourage you to take these commits, make any further changes you deem needed and submit a new PR.

v1v added a commit to v1v/github-api that referenced this pull request Jan 23, 2020
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.

4 participants