Fix respect throw option with invalid URLs
#2045
Conversation
Closes #1502 Notes: * Not sure if the verification should happen on Request() * The way throw is disabled on the test feels clunky, it might be a better approach
Codecov Report
@@ Coverage Diff @@
## master #2045 +/- ##
==========================================
+ Coverage 71.73% 71.81% +0.07%
==========================================
Files 179 179
Lines 14100 14124 +24
==========================================
+ Hits 10115 10143 +28
- Misses 3347 3349 +2
+ Partials 638 632 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I decided to add some duplication after all and check for Throw in both HTTP.Request and HTTP.Batch, for a couple of reasons: - Checking of options and especially logging should be done in functions "closer" to the user, and shouldn't concern internal functions. - Moving this check outside simplifies the internal functions, particularly batch request ones, since they don't have to handle the case of an empty ParsedHTTPRequest. Resolves #2045 (comment)
I decided to add some duplication after all and check for Throw in both HTTP.Request and HTTP.Batch, for a couple of reasons: - Checking of options and especially logging should be done in functions "closer" to the user, and shouldn't concern internal functions. - Moving this check outside simplifies the internal functions, particularly batch request ones, since they don't have to handle the case of an empty ParsedHTTPRequest. Resolves #2045 (comment)
imiric
force-pushed
the
fix/1502-throw-invalid-url
branch
from
0970583
to
283d4bd
Compare
I decided to add some duplication after all and check for Throw in both HTTP.Request and HTTP.Batch, for a couple of reasons: - Checking of options and especially logging should be done in functions "closer" to the user, and shouldn't concern internal functions. - Moving this check outside simplifies the internal functions, particularly batch request ones, since they don't have to handle the case of an empty ParsedHTTPRequest. Resolves #2045 (comment)
imiric
force-pushed
the
fix/1502-throw-invalid-url
branch
from
283d4bd
to
b894ab7
Compare
js/modules/k6/http/request.go
Outdated
| ) (*httpext.ParsedHTTPRequest, error) { | ||
| rt := common.GetRuntime(ctx) | ||
| state := lib.GetState(ctx) | ||
| if state == nil { | ||
| return nil, ErrHTTPForbiddenInInitContext | ||
| } | ||
|
|
||
| u, err := httpext.ToURL(reqURL.Export()) |
There was a problem hiding this comment.
Hmm I think goja.Runtime.ToValue() is a relatively expensive operation. We'd probably be better off if reqURL was of an interface{} type here, and we had a check like this here:
| u, err := httpext.ToURL(reqURL.Export()) | |
| if urlJSValue, ok := reqURL.(goja.Value); ok { | |
| reqURL = urlJSValue.Export() | |
| } | |
| u, err := httpext.ToURL(reqURL) |
This way we won't have to covert the url from a JS value to a Go one, and back to a JS value, and back to a go value.
There was a problem hiding this comment.
Probably true .... but given this common.Bind code (among other):
https://github.com/k6io/k6/blob/da5eebc638d9917cedfe98e232a7eed4cb69a157/js/common/bridge.go#L231-L236
I am not certain ... how much of a saving this will be 🤷
js/modules/k6/http/request.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| case string, httpext.URL: |
There was a problem hiding this comment.
Can you keep the comment from the previous default clause ?
There was a problem hiding this comment.
Hhmm but the comment doesn't make sense anymore since it was for the ToURL() call, which is now done in parseRequest().
Though would it be better to leave this as default and just pass anything to rt.ToValue()?
EDIT: I changed it to default in 39a0ed9.
js/modules/k6/http/request.go
Outdated
| return nil, err | ||
| } | ||
| state.Logger.WithField("error", err).Warn("A batch request failed") | ||
| return rt.ToValue(results), nil |
There was a problem hiding this comment.
Reading ToValue documentation has the following paragraph(near the very end, a few pages down):
Any other Go function is wrapped so that the arguments are automatically converted into the required Go types and the return value is converted to a JavaScript value (using this method). If conversion is not possible, a TypeError is thrown.
Which to me means that most of our usage of rt.ToValue is probably not necessary, including this one.
There was a problem hiding this comment.
@mstoykov WDYT about #2045 (comment)?
There was a problem hiding this comment.
Actually looking at common.Bind where we actually return func(call goja.FunctionCall) goja.Value and have to ToValue we apparently call it regardless ... so again even less needed. But Goja would've called it, either way, it's just that in this case we wrap this particular function call ... instead of goja doing it 🤦
https://github.com/k6io/k6/blob/da5eebc638d9917cedfe98e232a7eed4cb69a157/js/common/bridge.go#L246-L253
Also the part about multiple return values will not work ... and you can very easily make k6 panic if you write the wrong go code like func SomeThingThatGetsBinded() (string, string) ....
There was a problem hiding this comment.
Thanks for confirming this. I've seen both usages used, but it's good to know we don't have to convert it beforehand.
Resolved by 6b8cd30.
To minimize the amount of Go->JS->Go conversions. Resolves #2045 (comment)
To minimize the amount of Go->JS->Go conversions. Resolves #2045 (comment)
imiric
force-pushed
the
fix/1502-throw-invalid-url
branch
from
5fa6898
to
6b8cd30
Compare
js/modules/k6/http/request.go
Outdated
| return nil, err | ||
| } | ||
| state.Logger.WithField("error", err).Warn("Request Failed") | ||
| return &Response{Response: &httpext.Response{}}, nil |
There was a problem hiding this comment.
I am for this to be created from some function and to include more stuff as currently response.json() ends up as a panic instead of simple(r) exception, this will likely be true for everything.
Also I would argue that this should still have error and error_code set accordingly to what the problem was so you can detect what actually got wrong in the code.
There was a problem hiding this comment.
:-/ I still think it's strange to work with a Response object if a request was never made. IMO the correct behavior in this case would be to simply throw the parse exception and let the user handle it with try/catch as usual, in which case they'd have access to the error object. But since the Throw option is so old and we don't promote try/catch at all (it's nowhere in our docs 😞) we've backed ourselves into this corner where we have to return a fake-valid Response object. Would it be too late to bring up the discussion of deprecating Throw?
I'll see how I can improve things so that at least there are no panics, but this feels way off.
There was a problem hiding this comment.
Yeah... 😞 This is definitely one of the things that we have to improve in the new HTTP API... Not sure exactly how we should improve it, considering the context and requirements of load testing, but it has so many corner cases and footguns... 😞
There was a problem hiding this comment.
For me, this is a requirement for the merging of this PR
There was a problem hiding this comment.
@mstoykov Please take a look at the latest 2 commits.
This is crappy in several ways... One of which is that calling json() on the empty Response will fail parsing the body as JSON, and this failure does *not* respect the Throw option either and the iteration will be interrupted. But at least it doesn't panic... Resolves #2045 (comment)
| if state.Options.Throw.Bool { | ||
| return nil, err | ||
| } | ||
| state.Logger.WithField("error", err).Warn("A batch request failed") |
There was a problem hiding this comment.
Are we fine with this returning a different message than for a single request failure (Request Failed)? I thought it would be useful for troubleshooting to know it was a batch request, but maybe this should have the same message for consistency?
imiric
force-pushed
the
fix/1502-throw-invalid-url
branch
from
dd6f852
to
c87d6d1
Compare
imiric
force-pushed
the
fix/1502-throw-invalid-url
branch
from
c87d6d1
to
af755c4
Compare
|
Sorry for dismissing your review @na--, I kept finding some minor test tweaks ;) Hopefully should be good now. |
I decided to add some duplication after all and check for Throw in both HTTP.Request and HTTP.Batch, for a couple of reasons: - Checking of options and especially logging should be done in functions "closer" to the user, and shouldn't concern internal functions. - Moving this check outside simplifies the internal functions, particularly batch request ones, since they don't have to handle the case of an empty ParsedHTTPRequest. Resolves grafana/k6#2045 (comment)
To minimize the amount of Go->JS->Go conversions. Resolves grafana/k6#2045 (comment)
This is crappy in several ways... One of which is that calling json() on the empty Response will fail parsing the body as JSON, and this failure does *not* respect the Throw option either and the iteration will be interrupted. But at least it doesn't panic... Resolves grafana/k6#2045 (comment)
This is a continuation of #1853, with some changes as discussed in that PR.
Closes #1502