search

openfaas / golang-http-template

Golang templates for OpenFaaS using HTTP extensions
https://www.openfaas.com/
MIT License
106 stars 57 forks source link

Allow for customizing the read and write timeouts #22

Closed bmcustodio closed 5 years ago

bmcustodio commented 5 years ago

Description

Allows for setting the read and write timeouts using the read_timeout and write_timeout environment variables.

Closes #9.

How Has This Been Tested?

I've tested this by setting the aforementioned values to 10s (hence more than the default of 3s) and executing a piece of code that takes a while to complete:

2019/06/12 15:56:23 stderr: 2019/06/12 15:56:23 entered Handle
(...)
2019/06/12 15:56:27 exited Handle
2019/06/12 15:56:27 POST / - 200 OK - ContentLength: 0

Before this change, I would get something like the following:

2019/06/12 15:01:03 stderr: 2019/06/12 15:01:03 entered Handle
(...)
2019/06/12 15:01:07 exited Handle
2019/06/12 15:01:07 Upstream HTTP request error: Post http://127.0.0.1:8082/: EOF

How are existing users impacted? What migration steps/scripts do we need?

I am not really sure, since I am fairly new to OpenFAAS.

Checklist:

I have:

alexellis commented 5 years ago

Hi Bruno

Thanks for your contribution.

We need to be backwards compatible with integer format.

Please see how this is done in the openfaas-incubator/of-watchdog or openfaas/faas/watchdog and copy the approach.

Regards,

Alex

bmcustodio commented 5 years ago

Hi @alexellis,

Thanks for reviewing!

We need to be backwards compatible with integer format. Please see how this is done in the openfaas-incubator/of-watchdog or openfaas/faas/watchdog and copy the approach.

Are you referring to this function? If that is the case, I'd like to point out that, IMHO, that function has a few problems: it is neither compatible with integer values (as is being requested), nor is the default value observed properly whenever an environment variable cannot be parsed as a duration. I have put together an example here.

I will happily amend my commits in order to address your request, but I don't really feel like copying from https://github.com/openfaas-incubator/of-watchdog is the best approach. 🙂 Thoughts?

alexellis commented 5 years ago

In my comment, I asked you to look at both watchdogs.

Here's the code that I mean https://github.com/openfaas/faas/blob/master/watchdog/readconfig.go#L31

If it's not part of the of-watchdog codebase, then perhaps it should be, because we still have some users and old blogs / tutorial (that cannot be changed) that prescribe timeout values without a Golang duration added i.e. 60 -> 60s.

Does this make sense?

alexellis commented 5 years ago

but I don't really feel like copying from X

Why?

alexellis commented 5 years ago

Thanks for your comment. I've updated the code and raised an issue for the discrepancy you found in how of-watchdog parses user-supplied durations. Please feel free to work on the issue linked above. https://github.com/openfaas-incubator/of-watchdog/issues/64

alexellis commented 5 years ago

Hi @bmcstdio, I was under the impression from @ewilde that this was urgently needed. Please let us know if your issue has been addressed and works as expected.

https://github.com/openfaas-incubator/golang-http-template/pull/22#issuecomment-501794638

See comment above for contributing the fix to the other issue you found whilst investigating the solution.

Cheers,

Alex

bmcustodio commented 5 years ago

@alexellis it was indeed urgently needed - fortunately #23 fixed the problem, so thank you very much for that.