Skip to content
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.

Sign up for GitHub

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

[WIP] SDK overhaul #725

Closed
wants to merge 3 commits into from
Closed

[WIP] SDK overhaul #725

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c230ab1
Remove reflection from request and improve URL handling
dlsniper Jun 17, 2016
44815f0
Don't use reflection to compare expected values
dlsniper Jun 18, 2016
9cc004a
Remove needleess copy from handlers
dlsniper Jun 18, 2016
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
16 changes: 4 additions & 12 deletions aws/request/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import (
"io"
"io/ioutil"
"net/http"
"net/url"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -77,13 +75,10 @@ func New(cfg aws.Config, clientInfo metadata.ClientInfo, handlers Handlers,
p = "/"
}

httpReq, _ := http.NewRequest(method, "", nil)

var err error
httpReq.URL, err = url.Parse(clientInfo.Endpoint + p)
httpReq, err := http.NewRequest(method, clientInfo.Endpoint+p, nil)
if err != nil {
httpReq.URL = &url.URL{}
err = awserr.New("InvalidEndpointURL", "invalid endpoint uri", err)
return &Request{Error: err}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this can exit early here. It is possible for user's custom request handlers to depend on the request.Request.HTTPRequest value expecting it to be set. Returning early without the other fields could cause breaking changes because fields like HTTPRequest are no longer set. Its still an error condition, but exiting early may cause applications to panic instead of returning a API operation error.

I don't see any problem with getting ride of the url.Parse and letting NewRequest validate the URL. Both will report the error in Go 1.6. Neither 1.4 or 1.5 would return an error for invalid URLs such as http://example.com:80. For 1.4 and 1.5 the request would fail at transmission time instead of being caught early. #689.

Copy link
Author

Choose a reason for hiding this comment

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

My take on this is the following:

  • the function itself should have been returning a *Request, error pair from the beginning. It doesn't do so so I'd rather not break it yet.
  • regarding the issue in Trailing space in the Endpoint config parameter crashes the client #689, I don't see why this would make any difference to be honest. In both cases if there's an error there, the API call can't continue. If the developer can't provide a proper URL then it's ok for this to crash. We are also pretty close to Go 1.7 so there should be little to no incentive to support Go 1.4 (or really anything under 1.6 at this stage).

Copy link
Contributor

Choose a reason for hiding this comment

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

Both cases are an error, but if the SDK were to start panicking it would break user's applications where before only an error would of been returned.

The SDK's min supported version is 1.5, and plans to continue to support this version in the future. Go 1.4 is not officially supported by the SDK, but many users still use this version of Go, and we'd like to continue unofficial support for this version as long as possible. The SDK supporting these additional Go versions does not impact the overall performance of the SDK. This does introduce some limitations on what new stdlib features the SDK can use, but this has not introduced any issues as of yet.

}

r := &Request{
Expand All @@ -93,12 +88,9 @@ func New(cfg aws.Config, clientInfo metadata.ClientInfo, handlers Handlers,

Retryer: retryer,
Time: time.Now(),
ExpireTime: 0,
Operation: operation,
HTTPRequest: httpReq,
Body: nil,
Params: params,
Error: err,
Data: data,
}
r.SetBufferBody([]byte{})
Expand All @@ -115,14 +107,14 @@ func (r *Request) WillRetry() bool {
// and the parameters are valid. False is returned if no parameters are
// provided or invalid.
func (r *Request) ParamsFilled() bool {
return r.Params != nil && reflect.ValueOf(r.Params).Elem().IsValid()
return r.Params != nil
}

// DataFilled returns true if the request's data for response deserialization
// target has been set and is a valid. False is returned if data is not
// set, or is invalid.
func (r *Request) DataFilled() bool {
return r.Data != nil && reflect.ValueOf(r.Data).Elem().IsValid()
return r.Data != nil
Copy link
Contributor

@jasdel jasdel Jun 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The check for nil of Param and Data were to insure that a interface with a nil type value wasn't provided. Since r.Param != nil may fail with a false positive in the situation where a nil type value was set to the interface [1] [2].
Though in practice this could only happen if a user where to set the value of the r.Param or r.Data them selves since all operation's should always provided a valid value for two fields.

Are you seeing these two calls impacting the performance of the SDK? In the pprof's we've been running this hasn't registered.

The portion of the overhead which is impacted by reflection will be focused more on the (un)marshaling of the API operation's request/response data. As visible in #722.

}

// SetBufferBody will set the request's body bytes that will be sent to
Expand Down