cmd/swamr-smoke: fix waitToPushSynced connection closing

[janos]

No description provided.

[nonsense]

@janos what about this err? I am not sure rpcClient is guaranteed to be nil within this error.

I think the simplest solution is to keep the defer, but add a scope with a func() {} block.

[janos]

Should't this error mean that tpcClient is nil? At least that would be an expected behaviour to me. And it looks like that this is the case.

[nonsense]

@janos I checked rpc.Dial but the stack is too long. As a comparison, the logic in rpc/http.go is the following:

	respBody, err := hc.doRequest(ctx, msg)
	if respBody != nil {
		defer respBody.Close()
	}

	if err != nil {

I am not saying it is the same thing, but generally it is a good practice to always call Close() if the client is not nil, no matter the error.

[nonsense]

In src/net/http/client.go they also say:

// When err is nil, resp always contains a non-nil resp.Body.
// Caller should close resp.Body when done reading from it.

They don't give any guarantees on what happens if err != nil, and because of that the code I've seen at most projects, check for the return value and always calls Close() irrespective of err.

[janos]

Ok, lets be safe, but I think that if this kind of handling is expected from doRequest and Dial, it is a bad design.

[janos]

@nonsense, I thin that closing is not what the comment on http.Client is suggesting. It states that it is safe to ensure that it is not nil, not that it must be closed regardless of error value. All examples and documentation https://golang.org/pkg/net/http/ for that package defers closing after the error check.

[nonsense]

@janos i've read posts that state exactly the opposite - that clients should be closed regardless of the error value. i don't have hard feelings about this here, just wanted to raise it up from an engineering perspective as i found it interesting when i read it :) will try to find the blog post and share it.

[janos]

I am sure that if that is the case, net/http documentation would suggest it or at least there would be issues raised on golang/go repo. On the other side, if client.Do returns the error, no connection was made, so closing the body in order to close the connection is not needed. This should be also easily checked with a test.