ipfs / ipfs/go-graphsync

Graphsync Simplifcation Idea: Unblocking the ResponseManager

Open
#251 0 comments 0 reactions 1 assignee Claimed by @hannahhoward View on GitHub
effort/days exp/expert kind/architecture kind/discussion needs definition P2
Dominant language
Go
Stars
103
Forks
41
PR merge metrics
No merged PRs in 30d

Description

This is a meta "for discussion" issue

The basic architecture of the response manager is as follows: all public methods put messages into a buffered channel that is processed sequentially by an internal thread. (see https://github.com/ipfs/go-graphsync/blob/main/docs/architecture.md#actor-pattern-in-requestmanager-and-responsemanager for explanation)

We've now seen a couple deadlocks that the message queue gets filled, then the internal thread blocks because processing the next message somehow triggers sending a new message to the queue and gets blocked by that. Usually, it's a pretty indirect connection involving intermediate mutexes.

Simultaneously, we've seen race conditions that have arose from trying to do things outside this thread to keep things unblocked.

I think it's time to reexamine whether this is the best architecture.

My suggestion is we move from a global queue to per request queues -- so each ongoing request has its own message queue with its own go-routine processing messages.

The ResponseManager would then just manage a mutex locked list of requests that the go-routines would communicate with when they needed to modify the list (i.e. to close a request).

This would solve a number of problems and make code easier to understand -- currently, the state of a request is spread out across the response manager where it's tracked and the internals of the query executor where it actually runs.

The request go-routine would still have to communicate with the peertaskqueue to block its execution on the SImultaneousIncomingRequestLimit, but overall I think it would make things much simpler to reason about.

We still need to address buffering (see issue to address it globally -- https://github.com/ipfs/go-graphsync/issues/249), and honestly, I think we should just make the request message queues unbuffered.

To do:
- [ ] Spike on implementation
- [ ] Discuss and evaluate next steps
- [ ] Propose a final architecture

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.