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.

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

fix idle timeout issue #26

Merged
merged 1 commit into from
Apr 16, 2019
Merged

fix idle timeout issue #26

merged 1 commit into from
Apr 16, 2019

Conversation

rdallman
Copy link
Contributor

details of bug are here: nodejs/node#13391
docs here: https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_server_keepalivetimeout

tl;dr is node set a 5s idle timeout. this does what go and java both seem to
do (have not checked python or ruby, though from what I know about python,
we are not doing this either) and doesn't have an idle timeout. since in this
case we do kinda trust that the client is using an idle timeout (it's fn),
this seems like the right policy anyway (if fn dies is something to consider,
but the least of our worries is fdk conns in that case, and we are killing fn
spawned containers on startup too). it should also be noted the client (fn) is
only using 1 conn per container.

details of bug are here: nodejs/node#13391
docs here: https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_server_keepalivetimeout

tl;dr is node set a 5s idle timeout. this does what go and java both seem to
do (have not checked python or ruby, though from what I know about python,
we are not doing this either) and doesn't have an idle timeout. since in this
case we do kinda trust that the client is using an idle timeout (it's fn),
this seems like the right policy anyway (if fn dies is something to consider,
but the least of our worries is fdk conns in that case, and we are killing fn
spawned containers on startup too). it should also be noted the client (fn) is
only using 1 conn per container.
@rdallman
Copy link
Contributor Author

see fnproject/fn#1470 for the war story...

@denismakogon
Copy link
Member

For Python FDK we have 75 seconds for keep-alive timeout

@denismakogon denismakogon merged commit 6e79ea6 into master Apr 16, 2019
@denismakogon denismakogon deleted the fix-idle-bug branch April 16, 2019 08:35
@rdallman
Copy link
Contributor Author

@denismakogon can we bump up / turn it off?

@denismakogon
Copy link
Member

I don't think we can turn off keep-alive timeout, because it doesn't work like in node.js, in python 0 means that connection must be closed right away, that's why i picked bigger number.

I'd say if someone will face the problem then we'd need to bump keep-alive timeout. As an alternative would be good to let (from Fn) FDK know which idle timeout out to use.

@rdallman
Copy link
Contributor Author

i'm starting to think we should turn it off on the client side idle timeout and let the servers do it tbh... the client side we are closing them when we kill the container anyway, and the hole where we don't close them if fn dies is open whether we use idle timeout or not.

@rdallman
Copy link
Contributor Author

actually, that won't fix either, since we might still use a conn on the client side that's been closed on the server side. fun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants