Cleanup ctxgroup goprocess

[rht]

Close #1030

Pending waiting for goprocess to have SetTeardown()

[jbenet]

what i've been doing with goprocess is to make this a variable, not embedded type:

type Object struct {
  proc goprocess.Process
}

And then have a function that returns the process.

func (o Object) Process() goprocess.Process {
  return o.proc
}

the reason is that the Process interface is pretty large and embedding it dumps a bunch of objects on the user. maybe move to that? sorry for not describing it earlier.

[jbenet]

I'll wait till the SetTeardown and WithContextAndTeardown are in before CRing more. there may be other races to fix.

[rht]

How to godep update github.com/jbenet/goprocess? It errs "cannot find package x in any of $GOROOT, $GOPATH".

[jbenet]

@rht I updated it in #1392 should be able to just rebase on top

[rht]

Still want to know how it was done.

[jbenet]

@rht this is a typical flow.

# first, make sure my github.com/jbenet/goprocess dir is up to date
go get -u github.com/jbenet/goprocess
# or if that fails (dirty working dir or something):
cd ~/go/src/github.com/jbenet/goprocess && git fetch 

# tell godep to update github.com/jbenet/goprocess
cd ~/go/src/github.com/ipfs/go-ipfs
godep update github.com/jbenet/goprocess

# if godep errors with "cannot build package X" it usually means i don't
# have the version needed locally. One can try using `godep restore`
# but i like minimizing what godep touches, so i usually just update them manually:
go get -u github.com/jbenet/go-random
go get -u github.com/chriscool/go-sleep
...

# godep update should work now.
godep update github.com/jbenet/goprocess

# after updating, run `make vendor` to rewrite paths, if any have changed.
make vendor

# this _may_ remove godeps accidentally that shouldnt be removed. just dont
# commit those deletions, and just `git checkout Godeps/_workspace/...` them back

# selectively add only the bits we want:
# the vendored dir, and the updated line for the repo in the Godeps.json
git add -p Godeps/

[rht]

Thanks for writing that. It is ... very detailed and thorough.
(Though I doubt it will be rediscovered in google and will just stay in this corner of github)

i c so it is godep restore to nuke my problem, or:

go get -u github.com/jbenet/go-random
go get -u github.com/chriscool/go-sleep
...

If there is out-of-box way to go get each of the Godeps.json without that '...' (I mean one can just use jq/sed, but ...)

To sum up my surprise: why isn't godep already reading the $PWD/Godeps/_workspace in the first place (that it itself creates) in addition to $GOPATH and $GOROOT? Which it could have.

update: works now.

[rht]

@jbenet rebased and added Process() method, except for IpfsNode where it requires the process to be assigned externally.

[jbenet]

except for IpfsNode where it requires the process to be assigned externally.

what's the requirement? if lots of things call Close, so we can write a simple method like:

func (node *IpfsNode) Close() error {
  return node.proc.Close()
}

or something

[rht]

@jbenet fixed. SetTeardown can't just be glued to WithContext due to the special cases here.

[jbenet]

SetTeardown can't just be glued to WithContext due to the special cases here.

?

@rht i still think this is unsafe. it's a race that was there earlier.

Maybe this can fix it:

proc := goprocess.WithTeardown(node.teardown)
goprocessctx.WaitForContext(ctx, proc)

or maybe can make a function in https://github.com/jbenet/goprocess/blob/master/context/context.go like:

func WithContextAndTeardown(ctx context.Context, tf goprocess.TeardownFunc) goprocess.Process {
  if ctx == nil {
    panic("nil Context")
  }
  p := goprocess.WithTeardown(tf)
  go func() {
    <-ctx.Done()
    p.Close()
  }()
  return p
}

[rht]

Then is it likely that any code using SetTeardown is unsafe?

[jbenet]

For processes that spawn from a context or parent processes yes. (Which I guess is most cases where SetTeardown would be called).

We could actually fix this with a hack that may be semantically ok: if SetTeardown is called after the process is called, immediately call The teardown function. (Calling SetTeardown twice could be defined to be an error).

—
Sent from Mailbox

On Mon, Jun 22, 2015 at 3:23 AM, rht notifications@github.com wrote:

Then is it likely that any code using SetTeardown is unsafe?

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

[rht]

Hasn't this been done in https://github.com/jbenet/goprocess/blob/master/impl-mutex.go#L199 already?

[jbenet]

@rht

Hasn't this been done in https://github.com/jbenet/goprocess/blob/master/impl-mutex.go#L199 already?

i think that doesn't clear the parent's ptr to the processLink, so the processLink remains allocated.

[jbenet]

@rht +1 to all the golint / go vet changes. But hard to CR this with them in. could we separate those out into another PR and merge it first? (way easier to CR either of these in isolation + merge)

[rht]

i think that doesn't clear the parent's ptr to the processLink, so the processLink remains allocated.

I c, so what needs to be done is when the parent calls ClearChild() (https://github.com/jbenet/goprocess/blob/master/link.go#L72), it has to also unlink the child from the waitfors array.

[rht]

RFM.

This will be for a separate PR:

not necessarily, we just need to have a way to tell our parent we are dead, so that our parent removes the link.

A processLink itself should be removed whenever a ClearChild() or ParentClear() is called (actually this should just be Clear()).

[jbenet]

i think this is right, but @whyrusleeping can you check too?

[jbenet]

Hey @rht solid progress. almost there! I found another problem: #1385 (comment) that's my fault for not making CloseAfterContext() a func earlier. (which now makes it clear what's going on -- i.e. the word Close... appears there, so the safety concerns are raised.

[jbenet]

Also, make sure you rebase on latest master (haven't checked if this is rebased or not, just have merged many things recently)

[rht]

Also replaced anything with regex <-ctx.Done()\n.*close with CloseAfterContext.

[jbenet]

this LGTM. merging, thanks @rht !