Remove goprocess

[hannahhoward]

Goals

This change removes goprocess to reduce the learning curve for new people coming in to working on go-bitswap. While goprocess is a really cool library, it's non-standard, and it's partial implementation in go-bitswap, unique as I understand it to the IPFS codebase, increases confusion in the bitswap codebase. I remember losing at least a day to figuring out what it is and whether it should be there when I first started on go-bitswap. We're really not using it for anything special, other than to cancel some go routines, for which contexts are adequate.

We did have a broader conversation about go routine supervision in go-ipfs, but came to the conclusion that at least for the moment we're using just contexts.

Implementation

• Remove go-process, replace with context cancellation code where neccesary.

[Stebalien]

Unfortunately, this won't wait for bitswap to stop, that's really why we use goprocess. However, we can do the "go" thing and use a waitgroup.

That is, every where we would have called process.Go(...), call:

bs.wg.Add(1)
go func() {
defer bs.wg.Done()
}()

(and then bs.wg.Wait() in Close()).

[hannahhoward]

make sense (though I wonder what else we should be waiting to close -- sessions, wantmanager, etc)

will add.

[Stebalien]

Ideally, we'd wait for all workers.

[hannahhoward]

So, you know, if we need both a context and a waitgroup, and we need to cover all our workers -- if waiting for shutdowns across the board is really something we should be doing -- I wonder if we should just keep goprocess? I notice you have some recent commits on the repo -- may be we just start maintaining it? Also are we using it elsewhere? Maybe libp2p?

[hannahhoward]

Cause otherwise I'm gonna endup writing some code that looks like a mini-goprocess anyway.

[Stebalien]

I wonder if we should just keep goprocess?

I'd like to (a) switch to something simpler and (b) not leak it across abstraction boundaries.

And yes, we're also using it in go-libp2p.

(also, ipfs/kubo#5810 (comment))

[hannahhoward]

see monitor/monitor.go

[hannahhoward]

@Stebalien ok I built a little library to do this. The main goals were:

1. Keep it small (1 file)
2. Keep it flexible so it can be used without affecting API boundaries, and usable if we ever move off context as our main driver

The next step is to try applying it to the other long running routines in go-bitswap

see monitor/monitor.go

[Stebalien]

Given that we're already using goprocess elsewhere, I'm not sure if it's worth introducing a new abstraction into bitswap like this. If we're going to introduce a new abstraction, I'd rather prototype it in separate package, thoroughly test the API against all current uses of goprocess (not a small task).

[hannahhoward]

alright gonna close this PR for now.

I'm fine with prototyping something new -- but personally I feel like we need a clearer sense of our intent in a new package before we do it. I.e. what is wrong with go-process? Why can't we just improve it? Does it need to be better integrated with context?

[hannahhoward]

anyway that discussion can happen on the main issue.

[Stebalien]

but personally I feel like we need a clearer sense of our intent in a new package before we do it. I.e. what is wrong with go-process? Why can't we just improve it? Does it need to be better integrated with context?

Completely agree.