Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert thema.Library to *thema.Runtime #67

Closed
sdboyer opened this issue Sep 5, 2022 · 2 comments
Closed

Convert thema.Library to *thema.Runtime #67

sdboyer opened this issue Sep 5, 2022 · 2 comments
Labels
breaking change Breaks existing code enhancement New feature or request

Comments

@sdboyer
Copy link
Contributor

sdboyer commented Sep 5, 2022

thema.Library should be renamed to thema.Context. Its methods should also be converted to requiring a pointer, thereby requiring all existing function signatures to switch to *thema.Context.

I originally named it Library because i thought this was a nice nod to how we basically load a bunch of CUE "functions" in from the thema CUE package, then call them from the various Go methods within thema. And i wanted it to be a value, not a pointer, because i was hoping to make the zero value useful, similar to e.g. sync.Mutex.

However:

  • Library contains a cue.Context, and shuffling that thing around has ended up feeling like the main job of Library when actually using the Thema go package.
  • I'm not thrilled with calling it thema.Context because it sorta muddies the water by not having cancellation capabilities. But my real objection there is with how Go stdlib already muddied those waters by conflating variable bags together with cancellation. Asking people to learn a new term, Library, is a heavy lift for just resolving that ambiguity. Better to just fall in line with CUE itself and use thema.Context.
  • Implementing thema within Grafana has made it pretty clear that allowing an explicit nil is preferable for centralizing use of a single shared context. Adding nil to the accepted value space of a function signature allow the caller to make it clear that they are not specifying a value, and expecting the func impl to grab the central one instead. This could be accomplished without pointers by passing the zero value - thema.Library{} - but that feels like undocumented magic; expressing the absence of a value is, unfortunately, one of the main use cases for nil pointers in Go.

Thus: thema.Library -> *thema.Context.

@sdboyer
Copy link
Contributor Author

sdboyer commented Sep 23, 2022

Changed my mind - *thema.Runtime is better. Seeing a thema.Context and a cue.Context is just too much stutter, especially given that i think that the name is misleading in the first place.

@sdboyer
Copy link
Contributor Author

sdboyer commented Sep 23, 2022

There's a functional reason this is important, too - without the thing being a pointer type, we can't use a mutex to even try to guard against CUE current lack of thread safety due to data races from arising from the reference counter (see: cue-lang/cue#652 (comment))

@sdboyer sdboyer changed the title Convert thema.Library to *thema.Context Convert thema.Library to *thema.Runtime Sep 23, 2022
ishanjainn pushed a commit that referenced this issue Oct 27, 2022
Changes the name of thema.Library to thema.Runtime, makes it a pointer,
and tries to put mutex guards around any kind of concurrency that could
cause the CUE internals to blow up due to its naive reference counter
implementation.

Fixes #67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaks existing code enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant