add timeout in http request #20
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // set http connection timeout | ||
| switch timeout := options.RawGet(lua.LString("timeout")).(type) { | ||
| case lua.LNumber: | ||
| h.client.Timeout = time.Duration(timeout) * time.Second |
There was a problem hiding this comment.
This is not the correct way to implement a per request timeout. When implemented like this it would mean setting a timeout for one request would also affect requests after it that don't explicitly set another timeout.
The correct way is to use the Request.WithContext and a WithTimeout context.
| @@ -173,6 +174,11 @@ func (h *httpModule) doRequest(L *lua.LState, method string, url string, options | |||
| req.Header.Set(key.String(), value.String()) | |||
| }) | |||
| } | |||
| // set http connection timeout | |||
| switch timeout := options.RawGet(lua.LString("timeout")).(type) { | |||
| case lua.LNumber: | |||
There was a problem hiding this comment.
Can you also add support for LString using time.ParseDuration.
| @@ -67,6 +67,7 @@ func main() { | |||
| | query | String | URL encoded query params | | |||
| | cookies | Table | Additional cookies to send with the request | | |||
| | headers | Table | Additional headers to send with the request | | |||
| | timeout | Number | request timeout | | |||
There was a problem hiding this comment.
Please capitalize the first letter in "request" to match the style of the rest of the table.
There was a problem hiding this comment.
I'm extremely sorry that I just saw your reply now，I hope I can contribute to this project. I will follow your advice to modify pull request.
Merged
erikdubbelboer
pushed a commit
that referenced
this pull request
Jun 26, 2020
* Add timeout option for requests Based on an old PR at #20 that seems to have been deleted. The timeout option can be set to a number, which will be cast to an int and interpreted as seconds, or a string which will be parsed by time.ParseDuration. Add tests, one case should timeout and one that shouldn't. * Update readme with info about timeout option * Use WithContext() instead of Clone() Clone is not supported in older versions of Go and WithContext seems to work just as well. * Catch duration parsing errors, clean up imports Co-authored-by: Johannes Larsson <[email protected]>
fatalbanana
pushed a commit
to fatalbanana/gluahttp
that referenced
this pull request
Oct 18, 2023
* Add timeout option for requests Based on an old PR at cjoudrey#20 that seems to have been deleted. The timeout option can be set to a number, which will be cast to an int and interpreted as seconds, or a string which will be parsed by time.ParseDuration. Add tests, one case should timeout and one that shouldn't. * Update readme with info about timeout option * Use WithContext() instead of Clone() Clone is not supported in older versions of Go and WithContext seems to work just as well. * Catch duration parsing errors, clean up imports Co-authored-by: Johannes Larsson <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when I use the gluahttp in my project, I found that no param of http request timeout in the gluahttp.so I hope gluahttp can support this param. I have done the test.