Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fixes #1448 #1449

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

Closed
wants to merge 2 commits into from
Closed

Fixes #1448 #1449

wants to merge 2 commits into from

Conversation

tejksat
Copy link
Contributor

@tejksat tejksat commented Aug 12, 2020

Fixes #1448

@Test
public void closeStdinWithStdinOnce() throws Exception {
Assume.assumeTrue("supports stdin attach", getFactoryType().supportsStdinAttach());
Assume.assumeFalse("not fixed for Apache HttpClient 5.0", getFactoryType().equals(FactoryType.HTTPCLIENT5));
Copy link
Member

Choose a reason for hiding this comment

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

is it a WIP or you was unable to fix it for AHC5?

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 addressed this problem because my colleague at JetBrains stuck into the situation where our Python helper script misbehaved. And we use only OkHttp implementation.

However I dived into AHC5 implementation. It has pretty similar UnixDomainSocket class. But fixing only this class does not help because org.apache.hc.core5.http.impl.io.ContentLengthOutputStream.close() does not call close() method on underlying outputStream (which belongs to UnixDomainSocket that could be fixed) and, unfortunately,ContentLengthOutputStream class is situated in org.apache.httpcomponents.core5:httpcore5:5.0 artifact.

Here is the stacktrace where magic does not happen:

"docker-java-httpclient5-hijacking-stream-1739943297@6989" daemon prio=5 tid=0x3d nid=NA runnable
  java.lang.Thread.State: RUNNABLE
	  at org.apache.hc.core5.http.impl.io.ContentLengthOutputStream.close(ContentLengthOutputStream.java:92)
	  at org.apache.hc.core5.http.impl.io.DefaultBHttpClientConnection.sendRequestEntity(DefaultBHttpClientConnection.java:154)
	  at com.github.dockerjava.httpclient5.HijackingHttpRequestExecutor.lambda$executeHijacked$0(HijackingHttpRequestExecutor.java:82)
	  at com.github.dockerjava.httpclient5.HijackingHttpRequestExecutor$$Lambda$233.1769119455.run(Unknown Source:-1)
	  at java.lang.Thread.run(Thread.java:834)

Copy link

Choose a reason for hiding this comment

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

I have investigated the problem in AHC5 implementation. Although the org.apache.hc.core5.http.impl.io.ContentLengthOutputStream.close() method does not close the underlying OutputStream, it is possible to shut down the OutputStream in com.github.dockerjava.httpclient5.HijackingHttpRequestExecutor class right after conn.sendRequestEntity(fakeRequest); call:

if (conn instanceof ManagedHttpClientConnection) {
    ((ManagedHttpClientConnection) conn).getSocket().shutdownOutput();
}

However, for that to work, similar changes to the UnixDomainSocket class (under docker-java-transport-httpclient5) are needed because, by default, UnixDomainSocket.shutdownOutput() does nothing.

Copy link
Member

Choose a reason for hiding this comment

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

@mrtamm thanks for investigating, this is very helpful!

@tejksat will you be able to apply the changes as per @mrtamm's advice?

Copy link
Member

Choose a reason for hiding this comment

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

@@tejksat gentle ping :)

@stale
Copy link

stale bot commented Sep 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@bsideup
Copy link
Member

bsideup commented Mar 26, 2021

Superseeded by #1567

@bsideup bsideup closed this Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stdin remains open in Docker container after closing it on docker-java side via OkHttp
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.