fako1024 / httpc Goto Github PK

Right now, there is no default client-side timeout enforced (also refer to this quite commonly known discussion), and the WithTimeout() method only sets a context-based timeout (which will also take effect in case a request just takes a while, like downloading a large file).
While it's possible to specify a custom Transport using WithTransport, allowing the caller to set their own timeouts, there should probably be a) better defaults and b) some convenience methods to cover some common scenarios easily.

Hello. I just came across a case, where a client uses RetryBackOff and sets a retry interval of

httpc.Intervals{
			// retry three times before giving up
			1 * time.Second, 2 * time.Second, 4 * time.Second,
		},

The request call takes 7 seconds, no matter whether the context passed to it was cancelled (e.g. via CTRL-C):

# time gpctl config -f dummy.conf
^CError running command: failed to update goprobe's runtime configuration: Put "http://127.0.0.1:8145/config": context canceled

real	0m7.017s
user	0m0.015s
sys	0m0.001s

I would expect that the library honors the cancellation of ctx and also aborts its retry attempts.

Please let me know if this is enough info and whether help on this task is appreciated.

The code still uses deprecated calls to functions from io/ioutil. These should be removed / replaced with the recommended calls.

There seem to be two (probably unrelated) race conditions in test executions:

• In TestCertificateInstance:

2023/04/14 09:34:19 Unsolicited response received on idle HTTP channel starting with "HTTP/0.0 200 OK\r\n"; err=<nil>
    /home/fako/Develop/go/src/github.com/fako1024/httpc/tls_test.go:218: Get "https://127.0.0.1:10002/": readLoopPeekFailLocked: %!w(<nil>)

• In TestTimeout/with-timeout-method (seen in https://github.com/fako1024/httpc/actions/runs/4686844626/jobs/8305397977 on Windows):

httpc_test.go:103: Get "http://127.0.0.1/uiatbucacajdahgsdkjasdgcagagd/timeout": dial tcp 127.0.0.1:80: i/o timeout

The way RunWithContext is set up inside httpc, it prevents some important features which are available via NewRequestWithContext from net/http:

If body is of type *bytes.Buffer, *bytes.Reader, or *strings.Reader, the returned request's ContentLength is set to its exact value
(instead of -1), GetBody is populated (so 307 and 308 redirects can replay the body), and Body is set to NoBody if the
ContentLength
is 0.

This popped up testing github.com/els0r/goProbe trying to call global-query with goQuery, failing because the ingress in front of the global-query server redirected /_query to /_query/. httpc, or more specifically net/http wasn't following the request since the body isn't supplied to NewRequestWithContext.

Currently, there is at least one option that can only be used if another one is set (RetryBackOff() + RetryBackOffErrFn(), soon maybe even another one related to retries (see #17)). Right now, calling the latter without the former will not cause an error but simply do nothing (because no intervals are set). This should / could be handled more gracefully.

DoD

• Identify possible combinations of options / methods that might cause unclear situations for the caller
• Add error handling (i.e. fail on Run() with a useful error message if parameters are set inconsistently)
• Extend tests to cover such situations

As part of the rework to make sure that redirects are properly handled if a request body is supplied, check if the involved buffers can be reused for repeated calls of request.GetBody

In theory, http3 should be supportable quite easily by supplying a custom http.RoundTripper as indicated here: https://quic-go.net/docs/http3/client/

DoD

• Check options (explicit extension / feature or simply leave it to the user to specify custom client)
• Add tests (supported by gock?)

In order to improve performance, the base HTTP client implementation should be switched to github.com/valyala/fasthttp. Once a sufficient test suite exists, the current code base should be rewritten around said client implementation without breaking the existing API.

Just came across this library "by coincidence" 😏 . It makes the calls from a client definitely more concise, which is highly appreciated.

I'd like to propose a convenience function AuthDigest(digest string) *Request, which under the hood calls:

params := map[string]string{
    "Authorization": fmt.Sprintf("digest %s", digest),
}
return req.Headers(params)

or something along those lines.

It seems that the package depends on labstack/echo just for the sake of parsing a specific HTTP error struct which can easily be provided locally.

Relevant link: els0r/goProbe#111

DoD

• Replace HTTP error struct with local type
• Remove unnecessary dependency

Currently, performing a retry on error using RetryBackOff() is completely silent unless it fails on the last attempt. For debugging it would be helpful to be able to e.g. log a warning whenever a call is retried (e.g. showing after what delay and at which attempt). Since the package itself does not providing logging a handler would probably most suited (and generic enough to cover all sorts of scenarios).

DoD

• Add (optional) handler function that triggers on retries (provide attempt and delay(s))
• Add option to provide handler
• Add test

In case the caller sets the Content-Type Header manually while using one of the built-in encoders like EncodeJSON(), the headers gets duplicated instead of overwritten, leading to undefined behavior on some servers. This is a direct output of request.Headers from such a call:
Content-Type:[application/json application/json]

https://github.com/fako1024/httpc/security/dependabot/1