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 the OpenshiftClient connection leaks #7793

Merged
merged 3 commits into from
Dec 12, 2017

Conversation

davidfestal
Copy link
Contributor

What does this PR do?

Fix the OpenshiftClient connection leaks ... while reusing a common OkHttpClient instance as much as possible (not possible in some cases, like startExec(), that involve
InputStreamPumper and broken pipes).

What issues does this PR fix or reference?

This PR is related to the issue discussed here and here, as well as this initial issue of the OpenshiftConnector : https://issues.jboss.org/browse/CHE-180

It fixes these issues in a way that is compatible with the CHE-5-based OSIO multi-tenant server.

... while reusing a common `OkHttpClient` instance as much as possible
(not possible in some cases, like `startExec()`, that involve
`InputStreamPumper` and broken pipes).

Signed-off-by: David Festal <dfestal@redhat.com>
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@davidfestal davidfestal self-assigned this Dec 8, 2017
@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Dec 8, 2017
@codenvy-ci
Copy link

Can one of the admins verify this patch?

public OpenShiftClientFactory(
OpenshiftWorkspaceEnvironmentProvider workspaceEnvironmentProvider) {
this.httpClient =
HttpClientUtils.createHttpClient(workspaceEnvironmentProvider.getDefaultOpenshiftConfig());
Copy link
Member

Choose a reason for hiding this comment

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

@davidfestal not sure, but formatting does not look right from the first glance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it's formatted by the maven formatter plugin afaik

Copy link
Member

Choose a reason for hiding this comment

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

ok, cool - sorry than ;-)

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2017

ci-build

@codenvy-ci
Copy link

Build # 4379 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4379/ to view the results.

Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal
Copy link
Contributor Author

ci-build

1 similar comment
@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2017

ci-build

Signed-off-by: David Festal <dfestal@redhat.com>
@codenvy-ci
Copy link

Build # 4381 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4381/ to view the results.

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2017

ci-build

@codenvy-ci
Copy link

@davidfestal davidfestal merged commit abba419 into eclipse-che:master Dec 12, 2017
@benoitf benoitf added this to the 5.22.0 milestone Dec 12, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants