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

quick hack to prevent indefinite buildup of closenotify routines #2014

Closed
wants to merge 1 commit into from

Conversation

whyrusleeping
Copy link
Member

Until we get closeNotify fixed, this will help prevent these goroutines from hanging forever and building up in memory.

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

@jbenet jbenet added the status/in-progress In progress label Nov 28, 2015
@ghost
Copy link

ghost commented Nov 29, 2015

Will this properly kill a connection that is legitimately still open? (It's good if it does, just wondering if it'd still leak, just the other way around.)

@whyrusleeping
Copy link
Member Author

it will kill an active connection, yes. The only good way to fix this is to fix close notify :/

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
@rht
Copy link
Contributor

rht commented Dec 1, 2015

Actually this isn't a 'quick hack'. The timeout should exist but can be made longer when closenotifier is fixed.

There is only one test for nonexisting hash so far, https://github.com/ipfs/go-ipfs/blob/792da9d87587e28d65b98a28fa4cc2d20388ccca/test/sharness/t0220-bitswap.sh#L15, and even here the timeout is explicitly specified.
(if the daemon is up, then this hash will stay in the wantlist indefinitely even after the timeout)

Also there should be a way to check for active connections, e.g. http.StateActive for ConnState (in fact, this has been used to remedy the closenotifier stuff).

@@ -152,6 +153,9 @@ func (i internalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if cn, ok := w.(http.CloseNotifier); ok {
go func() {
select {
case <-time.After(time.Minute * 30):
// TODO: this is a hack to avoid these goroutines building up in memory
log.Warning("TODO: close notify needs to be fixed")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if a file is legitimately big enough + over a low bw conn to require 30min to stream? again, this needs to take into account the rate + progress. a timeout like this is ok when there havent been any bytes through (or too few)

@whyrusleeping
Copy link
Member Author

closing in favor of #2032

@jbenet jbenet removed the status/in-progress In progress label Dec 3, 2015
@rht
Copy link
Contributor

rht commented Dec 4, 2015

I think a timeout is still required for core.Resolve of a nonexistent hash.

@jbenet
Copy link
Member

jbenet commented Dec 4, 2015

@rht how do you distinguish (a) a nonexistent hash, (b) a hash normally available, just not now, (c) a hash hosted by a node very far away (latencies above 10s).

i think the "what to do" really depends on use case.

  • someone "wanting to download the thing" and not minding how long it takes would leave an app open like a bittorrent client.
  • someone building a webapp would want to know immediately if the thing isn't resolving.
    i'm strictly opposed to "always must timeout" because sometimes a user still wants to try resolving long term. (think of a torrent with no other users sitting lonely in the app, but eventually another user joins). but i do agree we need to set client-side timeouts by default more often and make setting infinite timeout a more explicit thing.

i also think that making sure all hashes being resolved have a context tied to the client properly will solve most problems: the issue is that a client issues "get X" and disappears. the context should be cancelled when the client disappears. it's like ssh <host> myprogram, myprogram will run until i disconnect. and i can also specify nohup.

@Kubuxu Kubuxu deleted the hotfix/close-notif-routine branch February 27, 2017 20:39
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.

3 participants