ensure hijackedConn implements CloseWrite function

[jim-minter]

Fixes #36516

[tophj-ibm]

ignore powerpc failure, unrelated and debugging.

[mrunalp]

@runcom @thaJeztah ping

[runcom]

LGTM ping @cpuguy83

[thaJeztah]

Thanks! Is is possible to have a test for this?

[thaJeztah]

PowerPC failures are not related

[thaJeztah]

nit: perhaps would be good to have the description of #36516 in the commit-message

[cpuguy83]

It would be good to only implement CloseWrite if the underlying conn does.

[cpuguy83]

Also we should think about removing this, I think?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@592a15b). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #36517   +/-   ##
=========================================
  Coverage          ?   35.13%           
=========================================
  Files             ?      613           
  Lines             ?    45639           
  Branches          ?        0           
=========================================
  Hits              ?    16036           
  Misses            ?    27462           
  Partials          ?     2141

[jim-minter]

Repushed adding a type assertion and improving the commit message. Beyond the type assertion I can't see a straightforward test case for this?

[cpuguy83]

It seems we need two types and decide which one to use based on if the underlying conn supports CloseWrite, otherwise the caller will end up doing something like:

cw, ok := hijack.(types.CloseWriter)
if ok {
    return cw.CloseWrite
}
return hijack.Close()

Which would lead to a leaked fd.

[thaJeztah]

Moved back to code-review per #36517 (comment), or could that change be done in a follow-up, @cpuguy83 ?

[cpuguy83]

It'd be best to handle it here rather than in a follow-up.

[jim-minter]

@cpuguy83 repushed with an additional type - ptal
@mrunalp @runcom fyi

[cpuguy83]

LGTM

[cpuguy83]

Minor nit, s/iff/if/.

[jim-minter]

I actually meant iff, as in "if and only if" :)

[thaJeztah]

@runcom still LGTY?

[runcom]

Yup!

[thaJeztah]

test-failing is unrelated

[kolyshkin]

Just to note that in a couple of other places we have something like this instead:

moby/client/hijack.go

Lines 26 to 33 in c3b3be5

	func (c *tlsClientCon) CloseWrite() error {
	// Go standard tls.Conn doesn't provide the CloseWrite() method so we do it
	// on its underlying connection.
	if conn, ok := c.rawConn.(types.CloseWriter); ok {
	return conn.CloseWrite()
	}
	return nil
	}

and this:

moby/api/types/client.go

Lines 133 to 139 in c3b3be5

	// CloseWrite closes a readWriter for writing.
	func (h *HijackedResponse) CloseWrite() error {
	if conn, ok := h.Conn.(CloseWriter); ok {
	return conn.CloseWrite()
	}
	return nil
	}

Looking at this, I have two conflicting thoughts:

1. we could use the same technique (i.e. call CloseWrite() if available, otherwise do nothing) for hijackedConn instead of implementing a separate hijackedConnCloseWriter.

2. However, the code quoted above may not work right in this scenario:
   

   moby/api/server/httputils/httputils.go

   Lines 36 to 47 in c3b3be5

   	// CloseStreams ensures that a list for http streams are properly closed.
   	func CloseStreams(streams ...interface{}) {
   	for _, stream := range streams {
   	if tcpc, ok := stream.(interface {
   	CloseWrite() error
   	}); ok {
   	tcpc.CloseWrite()
   	} else if closer, ok := stream.(io.Closer); ok {
   	closer.Close()
   	}
   	}
   	}

So, should we instead change the above two places to a hack similar to the one in this PR?

update: improve wording

[tonistiigi]

@kolyshkin I think so. But we probably don't need as complicated fix. For tlsClientCon it seems that it is only needed if CloseWrite is supported. For HijackedResponse I really do not understand why this method(or Close) is defined at all. Conn is public and all clients can check it directly. I'd just remove it (at least CloseWrite).

[cpuguy83]

We don't need tlsClientConn anymore since go1.8. I'm working on a PR to remove it but having some issues with my unit test hanging trying to setup the hijack.