FindProviders needs a timeout

[whyrusleeping]

In some 'real world' testing ive been find a large number of goroutines stuck waiting for 'FindProviders' requests to come back, looking into this, There is no timeout set on those requests (sent off into goroutines) so they wait forever for a request from the service. This causes an ugly memory leak of goroutines. What do you guys think an appropriate way to handle this is?

[btc]

Would passing the context down into the function work? Or does the callstack hit a wall where a message is passed over a channel?

[whyrusleeping]

Well, we do have a context passed down. but the passed in context is not guaranteed to ever be cancelled

[btc]

The language won't prevent you from extending FindProviders with its own timeout, but early termination is not really its call to make. That's the caller's responsibility.

There are three types of contexts

1. request-scoped contexts that only exist during a single request

2. system-scoped contexts that persist throughout the lifetime of the program (async workers respect these)

3. orphaned TODO/Background contexts These have no rightful place in the system except for the top of main.

Are we seeing instances of 2 and 3 along the callpath to this subsystem where they shouldn't be?

[whyrusleeping]

yeah, we are seeing 2 and 3 in the FindProviders calls

[btc]

yeah, we are seeing 2 and 3 in the FindProviders calls

I didn't see 2 or 3 in the FindProviders/GetProviders calls. Maybe I'm misunderstanding what you're seeing. In any case, I took a look at code and fixed a couple things that stood out.

1
Functions called by FindProviderAsync were not respecting the context. These two commits address this by ensuring all functions inside of FPAsync respect it to the best of their abilities:

• 0faf926
• cda4f1e

2
GetProviders was implemented using the promise pattern but with an unbuffered response channel. This causes the sender to hang when receivers aren't around to pick up the value. When receivers respect a context, this is a problem. A subtle detail, but one that is important for avoiding leaked goroutines. Addressed here:

• dec6dad

[jbenet]

The language won't prevent you from extending FindProviders with its own timeout, but early termination is not really its call to make. That's the caller's responsibility.

Agreed fully. This is particularly important when wanting to work in high-latency-low-bandwidth networks (mobile everywhere outside the US).

GetProviders was implemented using the promise pattern but with an unbuffered response channel.

Shouldn't this really be:

select {
case pm.getprovs <- gp:
  select {
  case resp := <- gp.resp:
    return resp
  case <-ctx.Done():
    return nil
  }

case <-ctx.Done():
  return nil
}

Or to make it more readable:

// send out async get providers request
select {
case <-ctx.Done():
  return nil
case pm.getprovs <- gp:
}

// receive a request response
select {
case <-ctx.Done():
  return nil

case resp := <- gp.resp:
  return resp
}

[btc]

Shouldn't this really be

resp is buffered and is locally/stack-allocated with this function as the only sender, so it acts like a dropbox. sender will never block. that's the crux of the change. i wish i knew how to make this clearer. perhaps through some conventional name? perhaps with another comment?

[jbenet]

resp is buffered and is locally/stack-allocated with this function as the only sender

no this function is the only reader. whatever is at the other side of pm.getprovs decides when to send into this channel. If it never sends anything, this goroutine blocks forever.

[btc]

no this function is the only reader. whatever is at the other side of pm.getprovs decides when to send into this channel. If it never sends anything, this goroutine blocks forever.

my mistake. will fix using this one:

// send out async get providers request
select {
case <-ctx.Done():
  return nil
case pm.getprovs <- gp:
}

// receive a request response
select {
case <-ctx.Done():
  return nil

case resp := <- gp.resp:
  return resp
}

[jbenet]

Is this fixed on master? pls reopen if not.