Skip to content

Commit

Permalink
Minor fixes and changes
Browse files Browse the repository at this point in the history
This commit contains some minor linting fixes.

It also removes a test log and renames the func test used in RoundTrip() to closeStream() and adds a warn log in faliure to close session and adds a comment detailing how we check if the response body is closed.

The todo by evanlinjin is also removed as we won't be reusing the streams as we are now closing them after every use.
  • Loading branch information
ersonp committed Jan 19, 2022
1 parent 4feab7f commit 44c6089
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 14 deletions.
2 changes: 1 addition & 1 deletion dmsgget/dmsgget_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func newHTTPClient(t *testing.T, dc disc.APIClient) *http.Client {
t.Cleanup(func() { assert.NoError(t, dmsgC.Close()) })
<-dmsgC.Ready()

log := logging.MustGetLogger(fmt.Sprintf("http_client"))
log := logging.MustGetLogger("http_client")
ctx, cancel := cmdutil.SignalContext(context.Background(), log)
defer cancel()
return &http.Client{Transport: dmsghttp.MakeHTTPTransport(ctx, dmsgC)}
Expand Down
3 changes: 2 additions & 1 deletion dmsghttp/examples_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/skycoin/dmsg/cmdutil"
"github.com/skycoin/dmsg/disc"
"github.com/skycoin/dmsg/dmsghttp"

"github.com/skycoin/skycoin/src/util/logging"
)

Expand Down Expand Up @@ -89,7 +90,7 @@ func ExampleMakeHTTPTransport() {
go dmsgC2.Serve(context.Background())
<-dmsgC2.Ready()

log := logging.MustGetLogger(fmt.Sprintf("http_client"))
log := logging.MustGetLogger("http_client")
ctx, cancel := cmdutil.SignalContext(context.Background(), log)
defer cancel()
// Run HTTP client.
Expand Down
20 changes: 9 additions & 11 deletions dmsghttp/http_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ func (t HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
hostAddr.Port = defaultHTTPPort
}

// TODO(evanlinjin): In the future, we should implement stream reuse to save bandwidth.
// We do not close the stream here as it is the user's responsibility to close the stream after resp.Body is fully
// read.
stream, err := t.dmsgC.DialStream(req.Context(), hostAddr)
if err != nil {
return nil, err
Expand All @@ -55,13 +52,13 @@ func (t HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
}

defer func() {
go test(t.ctx, resp, stream)
go closeStream(t.ctx, resp, stream)
}()

return resp, nil
}

func test(ctx context.Context, resp *http.Response, stream *dmsg.Stream) {
func closeStream(ctx context.Context, resp *http.Response, stream *dmsg.Stream) {
ticker := time.NewTicker(time.Second)
defer ticker.Stop()

Expand All @@ -72,12 +69,13 @@ func test(ctx context.Context, resp *http.Response, stream *dmsg.Stream) {
case <-ticker.C:
_, err := resp.Body.Read(nil)
log := stream.Logger()
log.Errorf("err %v", err)
if err == nil {
// can still read from body so it's not closed

} else if err != nil && err.Error() == "http: invalid Read on closed Body" {
stream.Close()
// If error is not nil and is equal to `http: invalid Read on closed Body`
// then it means that the body has been closed so we close the stream
if err != nil && err.Error() == "http: invalid Read on closed Body" {
err := stream.Close()
if err != nil {
log.Warnf("Error closing stream: %v", err)
}
return
}
}
Expand Down
2 changes: 1 addition & 1 deletion dmsghttp/http_transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestHTTPTransport_RoundTrip(t *testing.T) {
startHTTPServer(t, server0Results, lis)
addr := lis.Addr().String()

log := logging.MustGetLogger(fmt.Sprintf("http_client"))
log := logging.MustGetLogger("http_client")
ctx, cancel := cmdutil.SignalContext(context.Background(), log)
defer cancel()
// Arrange: create http clients (in which each http client has an underlying dmsg client).
Expand Down

0 comments on commit 44c6089

Please sign in to comment.