Skip to content
This repository has been archived by the owner on Feb 3, 2018. It is now read-only.

Incorporate context.Context into SourceManager #159

Open
sdboyer opened this issue Jan 26, 2017 · 5 comments
Open

Incorporate context.Context into SourceManager #159

sdboyer opened this issue Jan 26, 2017 · 5 comments

Comments

@sdboyer
Copy link
Owner

sdboyer commented Jan 26, 2017

We need to start using context.Context for handling cancellation-type behaviors within the SourceManager. There are two parts to this:

  • A context.Context should be passed to NewSourceManager(), and the cancellation channel there used to replace the exposed signal handling system. (It's still fine to have a helper func that sets up a Context for this purpose, though.)
  • SourceManager methods that touch disk or network - so, pretty much all of them - also need to take a context.Context, so that the caller has the option of controlling timeouts or forcing terminations.

There's no way this is feasible without a wider refactor of *SourceMgr to use channel-based brokers for all its activity, but that's fine - that absolutely needs to happen anyway. We can do all of that at the same time.

re: golang/dep#160

@sdboyer sdboyer changed the title Incorporate context.Context into SourceManager Incorporate context.Context into SourceManager Jan 26, 2017
@erizocosmico
Copy link
Contributor

I can tackle this if it's okay with you 😄

@sdboyer
Copy link
Owner Author

sdboyer commented Jan 28, 2017

Oh man, that'd be...amazing. 🎉 😄 🎉 But, the refactor here is likely to amount to a rewrite of significant portions of the SourceManager system. It's daunting enough that I'm worried about it turning into one of those dangerous, neverendering refactors...and I wrote the damn thing.

Don't get me wrong - I REALLY want other people to take ownership over parts of gps. (So, SO much). But this might not be the best place to start, as fully accomplishing it is likely to be the final step in a series of changes, including #84, #130, #150, and half a dozen more I haven't even had time to write up yet.

#84 is probably most directly related to this, but is more narrowly focused, and wouldn't be as affected by this bigger refactor I'm picturing. Maybe you could start by looking at that?

@erizocosmico
Copy link
Contributor

Yeah, totally, I'll start with #84. I've been taking a look at SourceMgr's code and you're right, it's a big daunting for a first time working on this codebase.

@sdboyer
Copy link
Owner Author

sdboyer commented Mar 21, 2017

(I'm now working on this in #196)

@fabulous-gopher
Copy link

This issue was moved to golang/dep#423

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants