karlkfi / inject

Dependency injection library for Go (golang)
Apache License 2.0
80 stars 7 forks source link

panic: runtime error: invalid memory address or nil pointer dereference #1

Closed ghost closed 9 years ago

ghost commented 9 years ago

Hi!

Nice package and good idea, but:

My source code:

//
// Application container
//
package main

import (
    "log"

    "flag"
    "net/http"

    "github.com/karlkfi/inject"

    "bitbucket.org/adknown/platform/backend/api/controller"
    "bitbucket.org/adknown/platform/backend/api/listener"
    "bitbucket.org/adknown/platform/backend/api/snapshot"
)

var (
    flagConfig = flag.String(
        "config",
        "/home/platform/etc/service-api.toml",
        "Adknown backend RESTful API service config",
    )
    graph inject.Graph
)

type ContainerInterface interface {
    Register()
}

//
// Snapshot container
//
type SnapshotContainer struct {
    BrowserSnapshot snapshot.Snapshot
}

func NewSnapshotContainer() ContainerInterface {
    snapshotContainer := &SnapshotContainer{}

    return snapshotContainer
}

func (this *SnapshotContainer) Register() {
    log.Printf("service_snap: registering snapshot services...")
}

//
// Controller container
//
type ControllerContainer struct {
    PingController controller.Controller
    FeedController controller.Controller
    LinkController controller.Controller
}

func NewControllerContainer() ContainerInterface {
    controllerContainer := &ControllerContainer{}

    graph.Define(
        &controllerContainer.PingController,
        inject.NewProvider(controller.NewPingController),
    )
    graph.Define(
        &controllerContainer.FeedController,
        inject.NewProvider(controller.NewFeedController),
    )
    graph.Define(
        &controllerContainer.LinkController,
        inject.NewProvider(controller.NewLinkController),
    )
    return controllerContainer
}

func (this *ControllerContainer) Register() {
    log.Printf("service_ctrl: registering controller services...")

    mux := http.NewServeMux()

    this.PingController.RegisterHandlers(mux)
    this.FeedController.RegisterHandlers(mux)
    this.LinkController.RegisterHandlers(mux)
}

//
// Listener container
//
type ListenerContainer struct {
    HttpListener  listener.Listener
    HttpsListener listener.Listener
}

func NewListenerContainer() ContainerInterface {
    listenerContainer := &ListenerContainer{}

    return listenerContainer
}

func (this *ListenerContainer) Register() {
    log.Printf("service_lsnr: registering listener services...")
}

//
// App container
//
type AppContainer struct {
    Listeners   ContainerInterface
    Snapshots   ContainerInterface
    Controllers ContainerInterface
}

func NewAppContainer() *AppContainer {
    graph = inject.NewGraph()
    appContainer := &AppContainer{}

    graph.Define(
        &appContainer.Listeners,
        inject.NewProvider(NewListenerContainer),
    )
    graph.Define(
        &appContainer.Snapshots,
        inject.NewProvider(NewSnapshotContainer),
    )
    graph.Define(
        &appContainer.Controllers,
        inject.NewProvider(NewControllerContainer),
    )
    graph.ResolveAll()

    log.Printf(">>> appcontainer.snapshots: %#v", appContainer.Snapshots)
    log.Printf(">>> appcontainer.listeners: %#v", appContainer.Listeners)
    log.Printf(">>> appcontainer.controllers: %#v", appContainer.Controllers)

    appContainer.Snapshots.Register()
    appContainer.Listeners.Register()
    appContainer.Controllers.Register()

    return appContainer
}

func (this *AppContainer) Run() {
    log.Printf("service_appl: running all the services...")

    flag.Parse()

    log.Printf("%+v", graph)
}

Sometimes it's everything ok, but eventually - panic (see controllers section)...

Alexander@Alexanders-iMac ~/Workspace/src/bitbucket.org/adknown/platform/backend/api
> $ ./build/darwin_amd64/bin/service-api -config ./build/darwin_amd64/etc/service-api.toml
2015/09/17 08:40:01 Adknown RESTful API service (0.1.0-77aaa71-dev)
2015/09/17 08:40:01 Copyright (C) 2015 Adknown Ltd. All Rights Reserved
2015/09/17 08:40:01 >>> appcontainer.snapshots: &main.SnapshotContainer{BrowserSnapshot:snapshot.Snapshot(nil)}
2015/09/17 08:40:01 >>> appcontainer.listeners: &main.ListenerContainer{HttpListener:listener.Listener(nil), HttpsListener:listener.Listener(nil)}
2015/09/17 08:40:01 >>> appcontainer.controllers: &main.ControllerContainer{PingController:(*controller.PingController)(0x4c8b38), FeedController:controller.Controller(nil), LinkController:controller.Controller(nil)}
2015/09/17 08:40:01 service_snap: registering snapshot services...
2015/09/17 08:40:01 service_lsnr: registering listener services...
2015/09/17 08:40:01 service_ctrl: registering controller services...
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x20 pc=0x27f0]

goroutine 1 [running]:
main.(*ControllerContainer).Register(0x82059ae70)
    /Users/Alexander/Workspace/src/bitbucket.org/adknown/platform/backend/api/app.go:84 +0x120
main.NewAppContainer(0x380b00)
    /Users/Alexander/Workspace/src/bitbucket.org/adknown/platform/backend/api/app.go:139 +0x581
main.main()
    /Users/Alexander/Workspace/src/bitbucket.org/adknown/platform/backend/api/api.go:23 +0x22b

As I understand, inject didn't instantiate controller instances...

2015/09/17 08:40:01 >>> appcontainer.controllers: &main.ControllerContainer{PingController:(*controller.PingController)(0x4c8b38), FeedController:controller.Controller(nil), LinkController:controller.Controller(nil)}

So, any suggestions or recommendations? Thanks! Alex

karlkfi commented 9 years ago

Well, the logs definitely show that all the snapshots, listeners and controllers are typed nils.

I can't really test my assumptions without code that compiles, but I think the issue is that your calling define with a pointer to a pointer of a pointer.

The example code and tests only use one or two layers of pointers, where the pointer can be resolved to a locally scoped variable, which is then populated by either a pointer or a primitive. But your code has &appContainer.Listeners which is a pointer to Listeners which is an interface, which accepts pointers (e.g. &ListenerContainer{}), but is itself a member of appContainer, which is a pointer to an AppContainer...

So there's a couple of ways you can resolve the references to make this work, but I think the most readable would be something like this:

    var (
        pingController controller.Controller
        feedController controller.Controller
        linkController controller.Controller
    )

    graph.Define(
        &pingController,
        inject.NewProvider(controller.NewPingController),
    )
    graph.Define(
        &feedController,
        inject.NewProvider(controller.NewFeedController),
    )
    graph.Define(
        &linkController,
        inject.NewProvider(controller.NewLinkController),
    )

    controllerContainer := &ControllerContainer{
        PingController: pingController,
        FeedController: feedController,
        LinkController: linkController,
    }

and this:

    var (
        listeners   ContainerInterface
        snapshots   ContainerInterface
        controllers ContainerInterface
    )

    graph.Define(
        &listeners,
        inject.NewProvider(NewListenerContainer),
    )
    graph.Define(
        &snapshots,
        inject.NewProvider(NewSnapshotContainer),
    )
    graph.Define(
        &controllers,
        inject.NewProvider(NewControllerContainer),
    )
    graph.ResolveAll()

    appContainer := &AppContainer{
        Listeners: listeners,
        Snapshots: snapshots,
        Controllers: controllers,
    }

Another alternative is to put in a dereference:

    graph.Define(
        &*controllerContainer.PingController,
        inject.NewProvider(controller.NewPingController),
    )

Let me know of one of those works for you.

On another note, I'm probably recommend against storing the graph as a "global", but I don't think that's your immediate problem.

It's probably a good idea to add constructor arguments to NewAppContainer and NewControllerContainer and pass in their dependencies. That way your constructors aren't dictating the implementations of their dependencies, which makes them brittle, less reusable, and less extensible. Instead, the main function should probably define which constructors to use for which variable.

Alternatively you might be able to pass the graph itself as a constructor argument and the constructors could lazily resolve only the objects that they need by type. You would have to supply the exact struct you needed, in this case, because you have multiple dependencies with the same interface. But there is still some benefit to only hardcoding the struct, rather than the constructor, because you're at least deferring the dependencies of your dependencies.

karlkfi commented 9 years ago

I'm gonna assume that resolved it. Feel free to reopen, if not.