Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

cmd/swamr-smoke: fix waitToPushSynced connection closing #1781

Merged
merged 2 commits into from
Sep 19, 2019
Merged

cmd/swamr-smoke: fix waitToPushSynced connection closing #1781

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b5ca25
cmd/swamr-smoke: fix waitToPushSynced connection closing
janos Sep 19, 2019
0ed0409
cmd/swarm-smoke: use closure for defer in waitToPushSynced
janos Sep 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions cmd/swarm-smoke/upload_and_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,6 @@ func waitToPushSynced(tagname string) {
time.Sleep(200 * time.Millisecond)

rpcClient, err := rpc.Dial(wsEndpoint(hosts[0]))
if rpcClient != nil {
defer rpcClient.Close()
}
if err != nil {
log.Error("error dialing host", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

@janos janos Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

continue
Expand All @@ -347,10 +344,12 @@ func waitToPushSynced(tagname string) {
synced, err := bzzClient.IsPushSynced(tagname)
if err != nil {
log.Error(err.Error())
rpcClient.Close()
continue
}

if synced {
rpcClient.Close()
return
}
}
Expand Down