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

webui keeps API server (daemon) from shutting down #28

Closed
torarnv opened this issue Apr 6, 2015 · 30 comments
Closed

webui keeps API server (daemon) from shutting down #28

torarnv opened this issue Apr 6, 2015 · 30 comments
Labels
help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws)

Comments

@torarnv
Copy link

torarnv commented Apr 6, 2015

The API server is served by braintree/manners, which keeps serving until all connections are closed.

Parts of the webui, such as polling for peers in the swarm, will open a connection and keep it open, but there doesn't seem to be any check for whether or not the server is in "shut down mode", which means the webui will keep the server alive forever.

On the ipfs daemon side there's no logging of why the server is kept alive, such as e.g. "Waiting for API connections to finish", and closing the tab with the webui in Chrome doesn't seem to immediately close the connection either, so the reason for the stuck server is quite opaque.

The only option is to forcefully close the daemon with another Ctrl+C, resulting in "Received another interrupt before graceful shutdown, terminating...", which can be a bit scary, as the user doesn't know what bad effects that might result in.

@STRML
Copy link
Contributor

STRML commented Apr 6, 2015

If the server publishes something to indicate it is closing connections, the polling could be stopped in the ui.

However this seems like a rather small problem - if the user is intentionally terminating the daemon, they likely have no need for the webui either and will close it.

This could alternatively be fixed on the API side by refusing new connections on shutdown but still completing established ones.

It appears there are other problems related to this in manners - braintree/manners#19

@torarnv
Copy link
Author

torarnv commented Apr 6, 2015

However this seems like a rather small problem - if the user is intentionally terminating the daemon, they likely have no need for the webui either and will close it.

This assumes the user knows that the webui is what's keeping the daemon from shutting down. Without --debug there's no indication of neither what's blocking shutdown (the API server), or in turn which connections the API server is waiting on.

It's also complicated by the fact that closing the webui tab (in Google Chrome) does not seem to instantaneously close the connection, so if the user suspects the webui is at fault, the "fix" of closing the tab doesn't have any effect.

This could alternatively be fixed on the API side by refusing new connections on shutdown but still completing established ones.

This is what's happening today. New connections are refused, but the swarm-polling connection is already set up, and is kept alive by the webui, so that doesn't help.

@whyrusleeping
Copy link
Member

Yeah, i've noticed this problem a few times before. I think we should move away from manners, as manners "key feature" is whats causing us issues. Lets just go back to using a normal go http server with a listener we can close ourselves.

@cryptix
Copy link

cryptix commented Apr 6, 2015

👍 on stdlib http server.

afaict it should be possible to also add graceful shutdown ourselves with listens to the context.

@jbenet
Copy link
Member

jbenet commented Apr 6, 2015

fine with either 👍

@travisperson
Copy link
Member

I think it would be best to keep a system in place that would gracefully shutdown the http interface by not accepting new connections if possible. It's just a bit nicer, but honestly I don't think it's a huge issue.

@jbenet
Copy link
Member

jbenet commented Apr 9, 2015

I think it would be best to keep a system in place that would gracefully shutdown the http interface by not accepting new connections if possible. It's just a bit nicer, but honestly I don't think it's a huge issue.

It's a good point.

is the problem with manners that polling requests hang it open?

the right way to do this would clearly label (in the go-ipfs commands code) which requests are allowed to complete, and which are interrupted.

Wait-- shouldn't the context cancellation make them all return asap? We may be looking at the wrong end of the problem.

@whyrusleeping
Copy link
Member

from the chrome side a polling request will not close immediately, it will stay open for a while. and manners wont exit until all its requests are finished

@jbenet
Copy link
Member

jbenet commented Apr 9, 2015

from the chrome side a polling request will not close immediately, it will stay open for a while. and manners wont exit until all its requests are finished

we should be able to close it early though. cancelling context should (not sure that it does) cancel everything all the way, and we should stop processing request input

@torarnv
Copy link
Author

torarnv commented Apr 9, 2015

we should be able to close it early though. cancelling context should (not sure that it does) cancel everything all the way, and we should stop processing request input

As mentioned, we currently stop accepting new connections. ipfs/kubo@6c64360b takes care of disabling keep-alive on existing connection so clients can detect the situation. What's left is dealing with those connections that still hang around, and I think drastically lowering the read timeout might solve that part, still need to play around with the code a bit.

@travisperson
Copy link
Member

We can set a timeout on the request object returned by the ipfs-node-api, and then call abort to cancel the request. This would solve this from the webui side. I'm not sure how it would affect the UI from a polling standpoint, some work would need to be done to look at that, it might be a drop in though.

The other side would be to kill connections on the server. Do we know what requests are polling and causing this issue?

@jbenet
Copy link
Member

jbenet commented Apr 23, 2015

Would be nice to fix this soon.

@jbenet jbenet added kind/bug A bug in existing code (including security flaws) help wanted Seeking public contribution on this issue labels Apr 23, 2015
@jbenet jbenet modified the milestone: 0.2.0 Apr 23, 2015
@vijayee
Copy link

vijayee commented May 12, 2015

I'm just about to start looking into this. Would it not be better to use web sockets vs long polling? Or was there some architecture specific choice that led to long polling.

@jbenet
Copy link
Member

jbenet commented May 13, 2015

i think the reasoning is that websockets requires... well, websockets. with longpolling, the API is the same as it would be normally, so it's easier for implementations to support it (just bare HTTP). it doesn't matter much either way yet, i think. If we were to add websocket support, it should be in addition to, not instead of.

@krl
Copy link
Contributor

krl commented May 16, 2015

Cannot reproduce this, is this still an issue?

@jbenet
Copy link
Member

jbenet commented May 16, 2015

I think it is-- its on the go-ipfs side though. (manners + long polling). Try it with the connections or events tab 


Sent from Mailbox

On Sat, May 16, 2015 at 1:12 PM, kristoffer [email protected]
wrote:

Cannot reproduce this, is this still an issue?

Reply to this email directly or view it on GitHub:
#28 (comment)

@krl
Copy link
Contributor

krl commented May 16, 2015

Hmm, still not locally reproducable. Have you double checked yourself if it's still the case?

@jbenet
Copy link
Member

jbenet commented May 16, 2015

I haven't, no. Maybe it's fixed! I'll post back here if I see it again.


Sent from Mailbox

On Sat, May 16, 2015 at 3:26 PM, kristoffer [email protected]
wrote:

Hmm, still not locally reproducable. Have you double checked yourself if it's still the case?

Reply to this email directly or view it on GitHub:
#28 (comment)

@torarnv
Copy link
Author

torarnv commented May 16, 2015

I belive it's still an issue, yes. The existing keep-alive sessions will keep the server up for 30-300 second depending on the browser. http://gabenell.blogspot.no/2010/11/connection-keep-alive-timeouts-for.html

@krl
Copy link
Contributor

krl commented May 18, 2015

I still cannot reproduce this, had three browsers pointed at /logs, killall ipfs took 1s. will put this on hold for now.

@krl krl removed this from the 0.2.0 milestone May 18, 2015
@vijayee
Copy link

vijayee commented May 18, 2015

I too had trouble reproducing this. But I did end up removing manners and replacing it with normal http requests and it appears to be working.

@jbenet
Copy link
Member

jbenet commented May 18, 2015

I think this got fixed in ipfs/kubo#1240

@jbenet
Copy link
Member

jbenet commented May 18, 2015

thanks @vijayee

@jbenet jbenet closed this as completed May 18, 2015
@jbenet
Copy link
Member

jbenet commented May 18, 2015

@torarnv reopen if you still find it a problem after pulling go-ipfs master

@torarnv
Copy link
Author

torarnv commented May 21, 2015

The server not shutting down is probably no longer an issue if manners was removed. But with manners removed, we're now killing all requests, which from an API POW is probably not ideal? The long-polling issue with manners was fixed in braintree/manners#19, perhaps we can get both?

@whyrusleeping
Copy link
Member

this is just my opinion, but if i knowingly press ctrl+c on my daemon, i dont care if any currently running requests get killed.

@torarnv
Copy link
Author

torarnv commented May 21, 2015

I guess that's true, if you are (personally) the only client of the web and ipfs gateway server. I don't know all the user-cases here, just thought I'd bring it up in case the web/ipfs server is expected to have clients other than the person running the daemon, and where there would be unfortunate side effects (worse than just losing the connection).

@whyrusleeping
Copy link
Member

yeah. thats a good point... I'm not sure of all the use cases here either.

@vijayee
Copy link

vijayee commented May 21, 2015

Would it be terrible if there was a websocket implementation of the commands api's? I feel like there should be a possibility of two way communication between the daemon and the interface if only to announce that the daemon is shutting down.

@jbenet
Copy link
Member

jbenet commented Jun 12, 2015

@vijayee a websocket implementation would be useful, too. just all the commands should still be accessible without websockets. so it should onyl be an optimization, not a replacement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

No branches or pull requests

8 participants