General go library design patterns

[jkline-trustle]

Thank you for your efforts to provide a go implementation of DBOS transact!

I request/recommend adopting some idiomatic go best practices for the library before committing to the 1.0 API contract.

• Remove global variables. Having a default client or registry that is global can make sense, similar to http.DefaultClient in standard library. But everything should be achievable without it. Imagine I will have 5 DBOS instances concurrently and in parallel for my unit tests.
• Separate instantiation from side effects.

package main
func main() {
    cfg, err := dbos.Config{logger, database url, admin server, etc, application version, executor id}
    myapp := dbos.New(cfg)
    err := myapp.Launch(context.Context) // connect to database, start background routines
    defer myapp.Shutdown(context.Context) // disconnect from database, shut down background go routines, stop the workflow scheduler
}

The dbos object should contain all the state, for example workflowQueueRegistry, workflow scheduler, gob encoder/decoder, logger, and not rely on separate global variables for each of these.

I recommend these resources for learning idiomatic go libraries:

• https://abhinavg.net/2022/12/06/designing-go-libraries/
• https://abhinavg.net/2023/09/27/future-proof-packages/

You can still provide convenient functions that default to a global, if customers want that convenience.

package dbos

func Launch(ctx context.Context, cfg Config) error {
    if globalDBOS != nil {
		return NewInitializationError("DBOS already initialized")
	}
   globalDBOS, err = New(cfg)
   if err != nil {
      return err
   }
   return globalDBOS.Launch(ctx)
}
func Shutdown(ctx context.Context) error {
    if globalDBOS == nil {
        return errors.New("Global DBOS instance is nil, cannot destroy")
    }
    defer func() { globalDBOS = nil }()
   return globalDBOS.Shutdown(ctx)
}

[maxdml]

Thanks @jkline-trustle for the input. For this

Separate instantiation from side effects

Agreed -- implemented in #36

The two other points -- having no globals and putting all globals in the DBOS object, are challenging for a two reasons:

1. Workflows and queues must be registered before DBOS starts (for recovery to work). Placing the registry within the DBOS object means forcing users to register workflows and queues in their main or init function, which will lead to bloated main functions.

2. Attaching all methods, even the non-workflow dependent ones, to the DBOS object means passing it around everywhere, which is going to make the programming experience difficult. A good example of this is when you want to call dbos.Recv() inside a workflow. You'd need to pass around the dbos object to your workflow functions (or make it global yourself)

Generally speaking, the main argument against globals is that it makes it difficult to perform dependency injection and mock for testing. We're planning to let users inject the system database, so they can generate a mock with, say, mockery, and initialize DBOS with it. How does that sound to you?

[jkline-trustle]

Workflows and queues must be registered before DBOS starts (for recovery to work).

Agreed, thus the importance of separating instantiation (aka Initialize) from start with side effects (aka Launch), like you implemented in #36.

I agree that bloated main files can be a problem. Some gophers choose solutions like fx to address that issue.

Passing the dbos instance in to workflow constructors will allow each constructor to register themselves. Or, the main can instantiate all the workflow objects and pass them to the dbos instance.

good example of this is when you want to call dbos.Recv() inside a workflow. You'd need to pass around the dbos object to your workflow functions (or make it global yourself)

Yes, that is exactly the behavior I'm asking for!
As some step enabled widget, I don't want to receive on the global dbos, I want to receive on the specific dbos instance that was passed in to my constructor.

The go standard library http.Client is a great example that implements the behavior I want and the short-cuts/sugar you are recommending.

All the behavior is on a Client struct.
There is also a global DefaultClient that is nothing but an instantiated Client, and there are global short-cuts like http.Get and http.Post that operate on the global DefaultClient.

You can do similar with having:

package dbos

var DefaultDbos

func Initialize(…) {
   if DefaultDbos != nil { 
        error, already initialized. Or make it a no-op.
   }
   // Probably should put a mutex around 
   DefaultDbos = NewDbosClient()
   return DefaultDbos.Initialize(…)
}

func Recv(…) {
  if DefaultDbos == nil {
      error, not yet initialized
  }
  return DefaultDbos.Recv(…)
}

If a consumer wants their go to use global dbos.DefaultDbos, I won't stop them.

I will kindly request that dbos globals be entirely avoidable please :)

I'm happy to set up a video chat if you want to discuss any ideas.

[jkline-trustle]

Passing the dbos instance in to workflow constructors will allow each constructor to register themselves.

Another option is to have workflow be an interface that other objects implement, and then pass the workflow objects to dbos.Initialize to get registered.
There are pros and cons and obviously that changes the instantiation order.
Since the workflow will need the dbos handle to for things like Recv I think it is preferable for workflow objects to also register themselves.

[maxdml]

One important consideration is that there should never be more than one DBOS instance -- hence the singleton. The DBOS instance operates at the process level and handling more than one efficiently would drastically complicate the framework. It's a very different use case than, say, the http package, where you can have many clients.

What use case do you have in mind for passing different DBOS objects around?

[jkline-trustle]

One important consideration is that there should never be more than one DBOS instance -- hence the singleton.

Fair and agreed. I have worked with other such situations in the past using the patterns described in https://abhinavg.net/2022/12/06/designing-go-libraries/.
That's one of the reasons that constructing the singleton should be the responsibility of a single package, and then the resulting object should be passed in to wherever it is needed so that other parts of the code don't try to construct it. It is possible to support singletons while still having the singleton be an object that maintains internal state.
It is just as much of a singleton to have

package dbos

var Singleton

as

package main
func main() {
   singleton = dbos.New()
}

The DBOS instance operates at the process level and handling more than one efficiently would drastically complicate the framework.

What specifically would be non-efficient?
I don't think go imposes any performance penalty on methods with receivers vs functions without receivers.

What would be more complicated? We're talking about the scope and receiver of methods and variables. Does that in any way change the fundamental algorithms or data stored in database?

I agree it is more complicated to design a maintainable easily usable library. I assert it is worth the effort :).

What use case do you have in mind for passing different DBOS objects around?

Exactly what you have described so far, including: Removing the need for global state, encapsulating behavior so it can be tested or replaced by different implementations, de-coupling the construction of the DBOS instance from the objects that consume it.

[maxdml]

Consider what happens when the singleton is Launched:

• A queue runner starts executing and dequeues tasks from Postgres based on the "executor ID" (which is typically exported through an env var). If multiple DBOS objects were to be started, we'd have to implement logic to support many executors living in a single process.
• A run of workflow recovery happens, likewise, on the process executor ID.
• The workflow scheduler starts executing scheduled workflow (that'd become 1 per instance again, undesirable)

Given there is no use case for having more than 1 DBOS object, we shouldn't complicate the library by supporting these.

Our plan to allow dependency injection for the system database and others during Initialize. This does not require the end-user accessing the singleton -- in fact, the end-user should never be concerned with managing the singleton, but rather decide when to start or stop it, which even goes against the idea of splitting initialize and launch...

[jkline-trustle]

Consider what happens when the singleton is Launched:

* A queue runner starts executing and dequeues tasks from Postgres based on the "executor ID" (which is typically exported through an env var). If multiple DBOS objects were to be started, we'd have to implement logic to support many executors living  in a single process.

* A run of workflow recovery happens, likewise, on the process executor ID.

You're just described the reason the executor ID is properly implemented as an input to the constructor to override the default value set from the global environment variable.

* The workflow scheduler starts executing scheduled workflow (that'd become 1 per instance again, undesirable)

The scheduler should be a data member of the main DBOS instance which you called executor.

Given there is no use case for having more than 1 DBOS object, we shouldn't complicate the library by supporting these.

It's literally the same code but re-ordered and spelled with a receiver instead of a global.
Define your top-level object that represents everything you think of when you say "singleton".

package dbos

type Executor struct {
    config Config
    workflowScheduler *cron.Cron
    logger *slog.Logger
    e executor
    systemDB SomeType
    appVersion string
    executorId string
    appID string
    adminServerPort int
    registry map[string]workflowRegistryEntry
    regMutex sync.RWMutex
}

// processConfig merges configuration from two sources in order of precedence:
// 1. programmatic configuration
// 2. environment variables
// Finally, it applies default values if needed.
func processConfig(cfg *Config) (*Config, error) {
    // keep it as written but add _all_ the parts that use os.Getenv, including appID and executorID

   if cfg.executorId == "" {
    cfg.executorId = os.Getenv("DBOS__VMID")
   }
   if cfg.executorId == "" { // environment was empty too
    cfg.executorId = "local"
  }

  // same for cfg.AppId, systemDBURL etc.
}

// New instantiates a DBOS executor.
// Most consumers should never need New and will have an easier time using the global singleton via dbos.Initialize.
func New(inputConfig Config) (*Executor, error) {


	// Create the system database
	systemDB, err := NewSystemDatabase(config.DatabaseURL)
	if err != nil {
		return newInitializationError(fmt.Sprintf("failed to create system database: %v", err))
	}
	logger.Info("System database initialized")

	// Set the global dbos instance
	dbos = &executor{
		config:   config,
		systemDB: systemDB,
                registry: make(map[string]workflowRegistryEntry)
	}

	return nil
}
 
// Initialize the global singleton.
func Initialize(inputConfig Config) error {
	if dbos != nil {
		fmt.Println("warning: DBOS instance already initialized, skipping re-initialization")
		return newInitializationError("DBOS already initialized")
	}
	dbos, err := New(inputConfig)
	return err
}

func (e *executor) RegisterWorkflow(...) {}

I've already described how func Launch(context.Context) error calls func (e *executor) Launch(context.Context) error with the global as a receiver.

Our plan to allow dependency injection for the system database and others during Initialize. This does not require the end-user accessing the singleton -- in fact, the end-user should never be concerned with managing the singleton, but rather decide when to start or stop it, which even goes against the idea of splitting initialize and launch...

You're describing injecting dependencies into DBOS, which is excellent. I'm describing when some other object depends on DBOS. (An example you've already mentioned, some object somewhere is going to self.dbos.Recv().)

[maxdml]

I spent time testing an application using DBOS by doing DI of a mock of the system database and agree this is not the right way because:

1. Users now have to understand systemDB internals to setup the right mocks
2. we'd have to make a bunch of internal stuff public from the DBOS package

This leads me to acknowledge the need for exposing an interface and having Initialize() return a concrete type implementing this interface, which will be mocked and injected to user functions made durable. (So you can mock stuff like dbosExecutor.Recv())

There is a big challenge in doing that, tho: many of our package-level methods are generics, which we absolutely want to keep as such to support compile-time type checking.

The DBOSExecutor interface bearing these methods would need to be generic too, which of course doesn't make sense in our case.

So these methods must remain global to the package and receive a mockable DBOS object (either through a context or a new parameter)

I am experiment with a few ways to do this. But there is obviously a core tension between generics and interfaces here.

[maxdml]

Most of these issues have been addressed in #44 #45 and #46. Didn't look into the gob encoder yet. Thanks for your feedback.