Refactor dig to separate graph handling and calling API #29

anuptalwalkar · 2017-04-28T18:05:06Z

access to the nodes map, duplicate code, and all the locking logic is getting difficult to work around. This refactor should help.

container.go -> user APIs to deal with package dig (Provide, Resolve, Invoke, etc)

pros - container doesn't know about nodes map.

package graph contains all the logic to insert object/constructors to the graph, read values from the graph and maintain locking throughout the usage.

pros- graph can manage its own lock

HelloGrayson

I see where you're going with this, but can you help me understand why we think we need locking?

HelloGrayson · 2017-04-28T18:11:30Z

graph/graph.go

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package graph


Let's move this package internal/ - I'd like to keep the API surface as small as possible.

+1 agreed. moving to internal.

glibsm · 2017-04-28T18:12:43Z

graph/graph.go

+)
+
+// Graph contains all Graph for current graph
+type Graph struct {


Im still thinking through this PR, but one thing is for certain: we don't want this to be a public type that users can interact with. It should either be private in the same package, or at the very least an internal package.

We have known for a while that options are coming: things like named instances, or private non-shared instances. I'd like for us to start thinking and working through those APIs, because only then we will be able to tell if this refactor is a good or bad thing.

Moving to internal package to avoid exposing graph to public.
With options coming, this will save us lot of headache of dealing with locks, maps, and reflect :)

anuptalwalkar · 2017-04-28T18:19:57Z

@breerly Locking is needed right now because we have APIs reading and iterating over the nodes map, Resolving of ProvideConstructor, and Invoke are iterating over the map and updating the map in the same call. nodes.value() call does the same recursively.

ghost · 2017-05-02T18:37:01Z

container.go

 				switch v.Type().Kind() {
 				case reflect.Ptr:
-					c.insertObjectToGraph(v, v.Type())
+					c.Graph.InsertObject(v, v.Type())


looks like we should strip type from the InsertObject, container should get it form the object itself.

good point. I'll update.

ghost · 2017-05-02T18:38:24Z

internal/graph/graph.go

+type Graph struct {
+	sync.RWMutex
+
+	nodes map[interface{}]graphNode


why it is a map[interface{}]graphNode not a map[reflect.Type]graphNode?

I don't want to change the map type in the same PR as it will get hard to read and change. Will follow up.

ghost · 2017-05-02T18:45:36Z

internal/graph/graph.go

+	}
+	for i := 0; i < argc; i++ {
+		arg := ctype.In(i)
+		if arg.Kind() != reflect.Ptr && arg.Kind() != reflect.Interface {


hmm, should the container care about it? Extending for slices and map will be hard then.

We are constructing values from the node, which container isn't aware of, so as of this PR graph is ideal place for building constructor arguments. I'll add/change it once slices and maps are introduced.

coveralls · 2017-05-02T18:47:44Z

Changes Unknown when pulling 53fa0da on refactor into ** on master**.

ghost · 2017-05-02T18:54:53Z

internal/graph/graph.go

+	n, ok := g.nodes[objType]
+	g.RUnlock()
+	if !ok {
+		return reflect.Zero(objType), fmt.Errorf("type %v is not registered", objType)


nit: just reflect.Value{}?

On the other hand, why return values, not pointers?

We set the pointer value of the provided object to the instance pointer in container. It makes sense since the function is defined to read value from the graph.

Also,"reflect.Zero returns a Value representing the zero value for the specified type." seems more appropriate to return as a default than reflect.Value{}?

There is no point of having a reflect.Value of specific type if there is an error :)
My comment was about unnecessary copying of reflect.Value that is going all around the graph. After digging through the code it looks like you'll need to change the graphNode interface to avoid it. May be another time then.

ghost · 2017-05-02T18:56:40Z

internal/graph/graph.go

+}
+
+// InsertConstructor adds the constructor to the Graph
+func (g *Graph) InsertConstructor(constr interface{}) error {


nit: usually constructor is abbreviated with ctor, constr is to close to const :)

ghost · 2017-05-02T18:57:40Z

internal/graph/graph.go

+
+// When a new constructor is being inserted, detect any present cycles
+func (g *Graph) detectCycles(n *funcNode) error {
+	l := []string{}


that's not needed, recursiveDetectCycles will work with nil too.

interesting..

coveralls · 2017-05-02T19:50:15Z

Changes Unknown when pulling b218032 on refactor into ** on master**.

anuptalwalkar added 2 commits April 28, 2017 10:57

Refactor dig to separate graph handling and calling API

3cbd191

fixed generated doc from readme

946bb06

HelloGrayson reviewed Apr 28, 2017

View reviewed changes

glibsm reviewed Apr 28, 2017

View reviewed changes

anuptalwalkar added 2 commits April 28, 2017 11:28

Moved graph to internal package

6717c5d

cleanup graph validation

4d9a6be

ghost reviewed May 2, 2017

View reviewed changes

simplified InsertObject method

53fa0da

ghost reviewed May 2, 2017

View reviewed changes

Addressed comments related to cycle and ctor naming

b218032

ghost approved these changes May 2, 2017

View reviewed changes

anuptalwalkar merged commit 53306d6 into master May 2, 2017

anuptalwalkar deleted the refactor branch May 2, 2017 20:31

hbdf pushed a commit to hbdf/dig that referenced this pull request Jul 14, 2022

Fix travis-ci badge path in the main README.md (uber-go#29)

f95a2db

Refactor dig to separate graph handling and calling API #29

Refactor dig to separate graph handling and calling API #29

Uh oh!

Conversation

anuptalwalkar commented Apr 28, 2017

Uh oh!

HelloGrayson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuptalwalkar commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

coveralls commented May 2, 2017 •

edited

Loading

coveralls commented May 2, 2017 •

edited

Loading