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

spec: define initialization order more precisely #57411

Closed
the-zucc opened this issue Dec 20, 2022 · 67 comments
Closed

spec: define initialization order more precisely #57411

the-zucc opened this issue Dec 20, 2022 · 67 comments

Comments

@the-zucc
Copy link

the-zucc commented Dec 20, 2022

Context

Package Initialization

When importing a package, it is initialized according to the sequence described in the Go specification. If that package has dependencies, it makes sense that these are initialized first, and that is what is described in the spec. In many instances, this is very useful and helps to write very succinct code, eliminating much boilerplate code.

Importing for Side Effects

In the Go specification, there is mention that it is possible to import packages for their side-effects:

An import declaration declares a dependency relation between the importing and imported package. It is illegal for a package to import itself, directly or indirectly, or to directly import a package without referring to any of its exported identifiers. To import a package solely for its side-effects (initialization), use the blank identifier as explicit package name:

import _ "lib/math"

Small Issue

Currently, the import order is predictable for packages with import relations. This is very useful and works flawlessly. However, as I've come to realize it, within a series of import statements in a single file, a package that is imported (for its side effects) early in the list of imports has no guarantee of early initialization. Consequently, the initializations are not ran in order, which makes results unpredictable at times.

Example Use Case - Mocking An Execution Environment Before Running Unit Tests

The package names here are not necessarily representative of what has caused this issue, they are just to explain the use case; let's say I have a package, packagea, that initializes a connection to a server, either in its init() or via variable initializers.

package somepackage_test

import _ "github.com/the-zucc/packageb" // this init() function sets an environment variable containing the server URL
import "github.com/the-zucc/packagea" // this package's init() function reads the environment variable

// Just imagine some unit tests here... :)

In this case, I've observed 2 things:

  • There's no guarantee that packageb will be imported and initialized before packagea -- in my case, the package imported the earliest was initialized at the end, effectively not propagating its side effects early enough in the program execution for them to apply to other package initializations, as the environment variable was not set which caused packagea to default to the "production code" server URL, and not the testing URL;
  • There isn't anything in the documentation or in the official Go specification referring to package initialization order within a series of import statements, or within a grouped import.

Proposed Solution

Enable some way of specifying that a package should be imported before any other package for its side effects. As an example, initialization behavior for grouped imports could remain the same, but for separate import statements, statement order could define the initialization order.

import _ "github.com/the-zucc/packageb" // this could be initialized first

import ( // and then the initialization behaviour could remain the same for this grouped import
    "github.com/the-zucc/packagec"
    "github.com/the-zucc/packagea"
)

EDIT: as mentioned in the comments, the proposed solution wouldn't work, unfortunately; however it still gives an idea of what a candidate implementation could look like.

@gopherbot gopherbot added this to the Proposal milestone Dec 20, 2022
@ianlancetaylor ianlancetaylor changed the title Proposal: Make Package Initialization Order Within a Series of Import Statements Predictable and Controllable proposal: make package initialization order within a series of import statements predictable and controllable Dec 20, 2022
@ianlancetaylor
Copy link
Contributor

CC @griesemer

@seankhliao
Copy link
Member

Given the widespread use of goimports and similar tools, the proposed solution won't work as it would be rewritten into a single block with sorted ordering.

More generally, what if a package does:

import _ "github.com/the-zucc/packageb" 

import (
    "github.com/the-zucc/packagec"
    "github.com/the-zucc/packagea"
)

while another does

import _ "github.com/the-zucc/packagec"

import (
    "github.com/the-zucc/packageb"
    "github.com/the-zucc/packagea"
)

or even different files in the same package?

@seankhliao
Copy link
Member

see also discussions in the other direction, pointing to perhaps the more common need to protect against people writing init functions

removing init #43731
control over defering init #48174
import without running init #38450

@the-zucc
Copy link
Author

Given the widespread use of goimports and similar tools, the proposed solution won't work as it would be rewritten into a single block with sorted ordering.

Depending on the implementation of this, goimports could need an update anyway.

More generally, what if a package does:

import _ "github.com/the-zucc/packageb" 

import (
    "github.com/the-zucc/packagec"
    "github.com/the-zucc/packagea"
)

while another does

import _ "github.com/the-zucc/packagec"

import (
    "github.com/the-zucc/packageb"
    "github.com/the-zucc/packagea"
)

or even different files in the same package?

That is a great question... couldn't this raise a syntax error ? Like a "pick a lane bud, you're drunk !" 😄

@seankhliao
Copy link
Member

packaged does the first one, packagee does the second
you import both packaged and packagee, but not any of packagea, packageb, packagec directly

are you just never allowed to import these two packages together?

@the-zucc
Copy link
Author

the-zucc commented Dec 20, 2022

packaged does the first one, packagee does the second you import both packaged and packagee, but not any of packagea, packageb, packagec directly

are you just never allowed to import these two packages together?

Without having thought about it too much (therefore don't take my word for it), I think I would simply raise a syntax error on both import lines (for packaged and packagee...) There is hardly any reason why one would want to have 2 different import orders, and if there was a reason, that reason doesn't exist as of today, because at the moment, package initialization order isn't guaranteed (and so we can eliminate that problem by raising a syntax error.)

@the-zucc
Copy link
Author

There is hardly any reason why one would want to have 2 different import orders, and if there was a reason, that reason doesn't exist as of today, because at the moment, package initialization order isn't guaranteed (and so we can eliminate that problem by raising a syntax error.)

Actually, I see a potential problem with applications with huge dependency trees with open source packages. One would want to take this into consideration if something is to be done...

@rittneje
Copy link

You should not connect to a server from init() as that violates the concept of writing testable code, as you have encountered.

Instead, packagea should have a proper constructor that gets invoked. Each unit test is then responsible for calling it. This establishes proper isolation between each test case, which is a very useful property.

Meanwhile, unit tests for other packages that depend on packagea should likely be using some interface instead of direct function calls, which allows for mocking.

@griesemer
Copy link
Contributor

See also #31636. You can force the init order by ordering the imports accordingly and into "blocks" so that they don't get re-ordered (example). Note that this is not prescribed by the language, it's an implementation behavior.

That said, it is much more robust to be explicit and call initialization functions explicitly if they cause side effects.

@the-zucc
Copy link
Author

You should not connect to a server from init() as that violates the concept of writing testable code, as you have encountered.

I guess you are right, there are probably other ways to get succint code without connecting to a server in the init() function. However the fundamental question regarding side effects is still of interest, for other use cases.

@seankhliao
Copy link
Member

it would be useful to have real usecases motivating these discussions.

@rittneje
Copy link

However the fundamental question regarding side effects is still of interest, for other use cases.

This proposal is only useful if package A's initialization is dependent on some other package B, despite the fact that A does not import B directly or indirectly. Writing code this way seems extremely confusing and error prone, especially because it will silently break if B imports A directly or indirectly, since then A would have to be initialized first no matter what.

@the-zucc
Copy link
Author

See also #31636. You can force the init order by ordering the imports accordingly and into "blocks" so that they don't get re-ordered (example). Note that this is not prescribed by the language, it's an implementation behavior.

That said, it is much more robust to be explicit and call initialization functions explicitly if they cause side effects.

I'm glad that in the example you provided, the same assumptions are made; however while testing this I experienced some irregularities.

  • Ordering imports as you have shown in the example does not seem to work for test packages;
  • The widespread goimports tool breaks intentional import rearrangements; if I force an import order, and run the code before goimports rearranges it, it works (for a main function at least.) But if I run goimports (as is done automatically in VS Code,) it rearranges it and it breaks the intended order. For that specific issue I'll probably raise an issue in goimports if it turns out to really be an issue

@griesemer
Copy link
Contributor

VSCode's goimport doesn't rearrange imports in different groups if they are separated by a blank line:

import (
	_ "b"
	// blank line
	_ "a"
)

(at least for me.)

That said, let me emphasize again: it's much better to explicitly call initialization functions that have important side effects if you want robust code.

@the-zucc
Copy link
Author

the-zucc commented Dec 21, 2022

VSCode's goimport doesn't rearrange imports in different groups if they are separated by a blank line:

import (
	_ "b"
	// blank line
	_ "a"
)

(at least for me.)

Thanks for chiming in; as I've mentioned, this doesn't seem to affect import order consistently for unit tests. It initializes packages in the proper order during normal execution, but when running unit tests, this order is not followed;

Let me share an example:

-- go.mod --
module somepackage

go 1.19
-- somepackage.go --
package somepackage

import (
	"os"
)

var ENV_VAR = os.Getenv("ENV_VAR")
-- mock/mock.go --
package mock

import "os"

const ENV_VAR_VAL = "localhost"

var _ = os.Setenv("ENV_VAR", ENV_VAR_VAL)
-- somepackage_test.go --
package somepackage_test

import (
	"testing"

	"somepackage/mock"
	_ "somepackage/mock"

	"somepackage"
)

func TestSomePackage(t *testing.T) {
	if somepackage.ENV_VAR != mock.ENV_VAR_VAL {
		t.Fail()
	}
}

If I run this code on my machine (i.e. run the unit test TestSomePackage(),) it shows that Go does not initialize somepackage/mock before somepackage.

@the-zucc
Copy link
Author

I also just confirmed that in the example above, running the tests in a separate test package causes the behavior to change:

// test/test_test.go
package test_test

import (
	"testing"

	"somepackage/mock"
	_ "somepackage/mock"

	"somepackage"
)

func TestTests(t *testing.T) {
	if somepackage.ENV_VAR != mock.ENV_VAR_VAL {
		t.Fail()
	}
}

In this case, the desired import order is the same, but the result is not the same, and the test passes.

@griesemer
Copy link
Contributor

What matters here is also in which order the files are presented to the compiler by the go command.

But I cannot but emphasize one more time: depending on these orders leads to extremely fragile code. If there are important side-effects that your code depends on, they should be encoded explicitly by calling respective initialization functions in the desired order.

@the-zucc
Copy link
Author

the-zucc commented Dec 21, 2022

That is true, and I agree; the practice should be avoided in most instances. However, the problem of reproducibility and consistency still remains. The commands I'm using to run unit tests is go test somepackage and go test somepackage/test, and so I'm not passing any files to the compiler "manually".

  • As it stands, simply moving a unit test file from the module's root package to a subpackage changes the import order. Should it be that way ?
  • According to the Go specification, packages can be imported for their initialization's side effects. However, these side effects are not guaranteed to run in a specific order during initialization, because this isn't part of the spec and is implementation-specific. In that case, wouldn't it be more coherent to simply discourage the use of that functionality in the Go spec ? or remove it from Go altogether, and recommend that initialization functions are called explicitly, like you are saying here ?

I'm just trying to understand why it was, that with all my careful research and attentive reading of the Go spec, I ended up believing that I can have some side effects propagated during the import process; and then ended up in the end realizing that the import order is inconsistent, and that the import for side effects functionality doesn't guarantee any import order and so should be avoided altogether.

If I am to import some package for its side effects, but can't have anything from the import/initialization process depend on it (implicitly), well then isn't the import for side effects functionality rendered useless, since we can't derive any assumption from it? I mean that, if the only code that can rely on side effects from imports is the one that sits in the package that is importing for side effects, couldn't the imported package (the one with the side effects) simply expose a method that is called in the init process of the importing package ?

I'm just thinking that if calling the initialization manually in my package is a better approach, then importing a package for its side effects isn't really all that useful and should probably be discouraged; am I missing something ?

Should a mention about this possible inconsistency be included in the spec, in the section where import for side effects is brought up?

@rsc
Copy link
Contributor

rsc commented Dec 21, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@ianlancetaylor
Copy link
Contributor

If we do anything here I think we should do the algorithm discussed in #31636. That issue was closed with a note in the CL that we should consider implementing the more deterministic algorithm later. CC @randall77

@ianlancetaylor
Copy link
Contributor

Specifically, the algorithm proposed in #31636 is "find the lexically earliest package [as sorted by import path] that is not initialized yet, but has had all its dependencies initialized, Initialize that package, and repeat."

@griesemer
Copy link
Contributor

@the-zucc The go command passes files to the compiler in lexical file name order.

@apparentlymart
Copy link

My sense of this so far has been that init is there as a measure of pragmatism because packages sometimes need to do some work to initialize their own internal state, for rare-but-unavoidable situations where a variable must be initialized by behavior that would not be permitted inline in the declaration.

A significant (but acceptable) consequence of that concession is that because init itself is arbitrary code it can call into other packages that may themselves require initialization, and so it is possible for one package's init to observe the side-effects of another if the ordering is not carefully controlled.

(I will concede that there are some packages in stdlib that seem to be intentionally designed around the existence of initialization side-effects, such as how database/sql deals with "drivers". I personally find that a rather regrettable design choice but it's not worth debating that both because it cannot change now, and because the problem that init functions can observe each other's side-effects remains even if nobody uses this register-on-import pattern.)

It seems to me that the existence of init requires there to be a well-defined inter-package init order in order to avoid dependency hell in larger programs that indirectly depend on packages that are dependent on one another and use init. Thankfully these problems don't seem to arise a lot in practice but when they do they are pretty confusing to debug and unclear how to resolve.

However, I'm very skeptical about the import order of dependencies of a particular package being significant. The author of a package fully controls what it directly depends on, and so can avoid using combinations of packages that don't make sense together, can directly import the packages whose side-effects they depend on, and avoid directly importing packages that contain known bugs (init-order-related or otherwise).


I share the same reaction to others in this discussion that having an init function have side-effects visible outside of the process seems like a design error in that package, and that the Go specification need not accommodate or encourage that design error.

However, if we put aside that particular part of the problem then we have an example where the init of "github.com/the-zucc/packagea" depends on the side-effects in the init of "github.com/the-zucc/packageb". If we assume that this interaction is unavoidable for the sake of this discussion, I would expect the one ending in packagea to itself import the one ending in packageb, to represent that it depends on that side-effect.

It feels like a bug to me that packagea depends on an import-time side-effect of packageb without declaring that dependency. With that package dependency declared, I believe the toolchain will already produce the correct ordering overall.

I feel skeptical that the specification needs to guarantee anything stronger than that.

@ianlancetaylor
Copy link
Contributor

@apparentlymart I am definitely sympathetic to that point of view. Still, for a counter-argument, see #31636 (comment) .

@the-zucc
Copy link
Author

the-zucc commented Dec 22, 2022

@the-zucc The go command passes files to the compiler in lexical file name order.

@griesemer Excuse my ignorance, and thank you for the clarification!

@apparentlymart I agree that relying on the import order for side effects being propagated is likely a very bad design choice if we're talking specifically about the example I mentioned earlier. However, wouldn't bringing consistency to import initialization order lessen that fact? Wouldn't it be "okay" (not saying it would be excellent, far from that) to be able to rely on a behavior that we can predict and that is consistent?

Also, the whole idea of relying on import order for side effect propagation, again, starts with the mention in the spec:

To import a package solely for its side-effects (initialization), use the blank identifier as explicit package name:

import _ "lib/math"

This is inconsistent with the very valid principle that everyone has outlined here, that one shouldn't rely on a specific package initialization order for side effect propagation.

In my point of view, it comes back to the same options that were outlined back in 2019 in #31636 (comment), but with a twist: in addition to any decision regarding the options previously outlined, why wouldn't importing for side effects just be discouraged in the spec ? Or does this fall out of the scope of such a specification?

I'm thinking, something can be done that is similar to what is done with dot imports, where they are allowed but discouraged, and it clearly outlines it in the standard linting/syntax highlighting tools for Go in VSCode (staticcheck). This could be the case for imports for side effects. Just throwing that out there.

@rittneje
Copy link

This is inconsistent with the vely valid principle that everyone has outlined here, that one shouldn't rely on a specific package initialization order for side effect propagation.

This is inaccurate. You should not rely on a specific initialization order when there is no explicit dependency between the two packages. That is:

  1. If package A imports package B, then it is perfectly reasonable for A to rely on B getting fully initialized first.
  2. If package A does not import package B, then you should not assume anything about the relative order in which they are initialized. This is not just because the Go specification does not currently prescribe the order, but also for humans it is far easier to reason about what happens when it is only a function of what packages A and B themselves say. If it is contingent upon what some other arbitrary package C says (as is the case in your original proposal based on the order in the import block), then you have to know about the entire application in order to reason about the behavior.

The suggestion to have the language spec mandate that packages be initialized in lexicographic order when there is a "tie" is useful to eliminate currently undefined behavior that could cause grief as the toolchain evolves. However, it would still be ill-advised to rely on this behavior. For example, suppose I have two packages - "mypackage/one" and "mypackage/two". In this hypothetical future, assuming neither imports the other, "mypackage/one" will be initialized first, and "mypackage/two" could implicitly rely on this fact. But then suppose some developer makes changes that cause "mypackage/one" to import "mypackage/two". Now the import order will silently be reversed, causing the application to break in some way. And to the developer making the change, the reason for the failure may not be immediately obvious. If however the original dependency had been made explicit by having "mypackage/two" import "mypackage/one", then the aforementioned change would have resulted in a clear compile-time failure due to the import loop.

@the-zucc
Copy link
Author

This is inaccurate. You should not rely on a specific initialization order when there is no explicit dependency between the two packages.

@rittneje If there is an explicit dependency between the packages, the init order between them is predictable, and so your reasoning is not valid for the concerns I am expressing here.

@dmitshur dmitshur added this to the Go1.21 milestone Mar 4, 2023
@rsc rsc reopened this Mar 15, 2023
@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

Rolled back in https://go.dev/cl/474976 due to problems in Google's internal test suite and perhaps also the aix builder.

@gopherbot
Copy link

Change https://go.dev/cl/478916 mentions this issue: cmd/link: establish dependable package initialization order

@randall77
Copy link
Contributor

Retry in CL 478916.

Google internal ran into two issues with this change. I will explain one; the other is very similar.

The problem is a undefined ordering between flags.String (or other variants that make a flag) and flags.Set.

Package a does

package a
import "flag"
var myFlag = flags.String("myFlag", "defaultValue", "description")

Package b does

package b
import "flag"
func init() {
    flags.Set("myFlag", "otherValue")
}

Typically package b is a test package and is modifying the behavior of a for testing purposes. For example, pointing a to a test database instead of the production database.

Critically for the issue at hand, b does not import a directly or indirectly. This is unusual, as normally when b is using the functionality of a it imports a, at least indirectly. But there are cases where even an indirect dependence is missing, like if a registers itself with http.ServeMux.Handle and b then sends a http request to that ServeMux.

Without even an indirect dependence, the initialization order is unspecified (before this CL, that is). If the initialization of a happens first, everything is ok. If the initialization of b happens first, then Set returns an error because the flag isn't defined yet. And then the calling code ignores the returned error, and is blissfully unaware that its Set call didn't do what it expected.

The fundamental problem here is that we're trying to establish dependence by name (a string) and not by an actual language construct reference. It would be better if the only way to set a flag was to get the address of it somehow (which would require exporting it, at least in test mode). In any case, that ship has sailed with how flags.Set is currently defined.

We're working on fixing some of these implicit ordering dependence things inside Google so we can retry the CL. The question here is, how much do we expect external code to run into this? Anything we can do to mitigate it?

One option to consider is having flags.MustSet that panics instead of returning an error.

@ianlancetaylor
Copy link
Contributor

A possibility: if flag.Set is called on a non-existent flag, we record that flag and return an error. If we later see a definition of that flag, we panic. I think this might be OK on the theory that no reasonable program would expect to ignore an error from flag.Set and then later define that flag. That seems like a clear program error.

@gopherbot
Copy link

Change https://go.dev/cl/480215 mentions this issue: flag: panic if a flag is defined after being set

gopherbot pushed a commit that referenced this issue Apr 21, 2023
As part of developing #57411, we ran into cases where a flag was
defined in one package init and Set in another package init, and there
was no init ordering implied by the spec between those two
packages. Changes in initialization ordering as part of #57411 caused
a Set to happen before the definition, which makes the Set silently
fail.

This CL makes the Set fail loudly in that situation.

Currently Set *does* fail kinda quietly in that situation, in that it
returns an error. (It seems that no one checks the error from Set,
at least for string flags.) Ian suggsted that instead we panic at
the definition site if there was previously a Set called on that
(at the time undefined) flag.

So Set on an undefined flag is ok and returns an error (as before),
but defining a flag which has already been Set causes a panic.  (The
API for flag definition has no way to return an error, and does
already panic in some situations like a duplicate definition.)

Update #57411

Change-Id: I39b5a49006f9469de0b7f3fe092afe3a352e4fcb
Reviewed-on: https://go-review.googlesource.com/c/go/+/480215
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/498755 mentions this issue: doc/go1.21: document clear builtin and init order changes

gopherbot pushed a commit that referenced this issue May 27, 2023
Also move all the language changes to the same part of the release notes.

For #56351
For #57411

Change-Id: Id1c51b5eb8f7d85e61a2ae44ee7d73bb13036631
Reviewed-on: https://go-review.googlesource.com/c/go/+/498755
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/499278 mentions this issue: doc: document new panic behavior of flag definition after Set

gopherbot pushed a commit that referenced this issue May 30, 2023
For #57411

Change-Id: I56c112bb03dde24c2e2643c9b72ce06158a8e717
Reviewed-on: https://go-review.googlesource.com/c/go/+/499278
TryBot-Bypass: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Keith Randall <khr@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/501696 mentions this issue: spec: document new program initialization process

gopherbot pushed a commit that referenced this issue Jun 13, 2023
For #57411.

Change-Id: I94982d939d16ad17174f801cc167cc10ddc8da30
Reviewed-on: https://go-review.googlesource.com/c/go/+/501696
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Bypass: Robert Griesemer <gri@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
@hunshcn
Copy link

hunshcn commented Nov 28, 2023

So now there is no way to force initialization order? It seems sad.

@griesemer
Copy link
Contributor

@hunshcn Initialization order is still under user control. If you have a concrete concern, please file an issue explaining your concern with a reproducible case or clear example. Thanks.

@hunshcn
Copy link

hunshcn commented Nov 29, 2023

@hunshcn Initialization order is still under user control. If you have a concrete concern, please file an issue explaining your concern with a reproducible case or clear example. Thanks.

Can you guide me? I tried

package main

import (
	_ "issue/packageB"
)

import (
	"fmt"

	_ "issue/packageA"
)

func main() {
	fmt.Println("ok")
}

and

import (
	"fmt"

	_ "issue/packageB"

	_ "issue/packageA"
)

packageA/B content is simple.It has one file named a.go

package packageB

import "fmt"

func init() {
	fmt.Println("packageB/a.go init")
}

In the above two import methods, PackageA is executed before PackageB.

@randall77
Copy link
Contributor

If you want packageB to be initialized before packageA, then do, in packageA

import _ "issue/packageB"

@hunshcn
Copy link

hunshcn commented Nov 29, 2023

If you want packageB to be initialized before packageA, then do, in packageA

import _ "issue/packageB"

This is the simplest case. In a normal scenario, packageA/B may be a third-party package.

@randall77
Copy link
Contributor

Can you be specific about the two third-party packages you're having trouble with?

@hunshcn
Copy link

hunshcn commented Nov 29, 2023

Can you be specific about the two third-party packages you're having trouble with?

I don't think the specific package is critical. In go1.20, adjusting the import order in main can affect initialization order. go1.20 cannot

But for your question, I will also make an example to describe the scene. Please give me some time.

@hunshcn
Copy link

hunshcn commented Nov 29, 2023

@randall77 Thank You for Your Time

in my project (a k8s controller)

there are two workqueue metric registration, "sigs.k8s.io/controller-runtime/pkg/metrics" and "k8s.io/component-base/metrics/prometheus/workqueue".

The simultaneous import of the two packages is not explicitly declared by me, I just want to use "sigs.k8s.io/controller-runtime/pkg/metrics". "k8s.io/component-base/metrics/prometheus/workqueue" from the deep dependency

They both registry workqueue metric, but workqueue provider can only registry once. So the import order becomes very important.

I have no idea to deal with this problem on go 1.21 at present. In go 1.20, I only need to import "sigs.k8s.io/controller-runtime/pkg/metrics" at the top of package main to solve it.

@randall77
Copy link
Contributor

I'm a bit confused by your answer. You quote two packages that you want to initialize in a specific order, but I don't understand why you need them initialized in a specific order.

My best guess: are they both calling into k8s.io/component-base/metrics/legacyregistry.MustRegister, and the ordering of those calls matters?

@hunshcn
Copy link

hunshcn commented Nov 29, 2023

@randall77 You are very wise. Just an hour ago,I located the citation exception through dependency analysis. Two years ago, our team members accidentally imported k8s.io/apiserver/pkg/server, and after a lengthy chain import, "k8s.io/component-base/metrics/promises/work queue" was mistakenly imported. This erroneous introduction was not discovered until this change. (Although this problem was discovered before, it was not deeply located, and adjusting the import order was enough to solve the problem.)

However, this is also an example, and defining initialization order may be a potential requirement.

@randall77
Copy link
Contributor

I think I still need more details to understand.
What, exactly, is the failure you are seeing? What command did you run, what did you expect as a result, and what did you see instead?

@randall77 You are very wise.

Does that mean I'm right about legacyregistery.MustRegister? If that's the case, I still don't understand the problem. What about that registration is order-dependent?

Just an hour ago,I located the citation exception through dependency analysis.

I don't understand what this means.

Two years ago, our team members accidentally imported k8s.io/apiserver/pkg/server, and after a lengthy chain import, "k8s.io/component-base/metrics/promises/work queue" was mistakenly imported. This erroneous introduction was not discovered until this change.

By "this change", you mean the one you're currently working on? So there was a latent bug for 2 years that you're now tripping up on? Shouldn't that latent bug be fixed instead of papering back over it by reordering initialization?

@hunshcn
Copy link

hunshcn commented Nov 29, 2023

By "this change", you mean the one you're currently working on? So there was a latent bug for 2 years that you're now tripping up on? Shouldn't that latent bug be fixed instead of papering back over it by reordering initialization?

No, after discovering what I said, I fixed it.

I mean, similar things may happen in other scenes. Of course, I provided a bad example. I'm sorry.

@randall77
Copy link
Contributor

Ok, thanks for the discussion.
I think we're in agreement that there could be situations where initialization order matters and people would want to control it. But barring an actual example where this happens, it is hard to justify changing the current semantics at this point. Particularly, I would want to see a situation where there isn't some other means of achieving the desired result.

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

No branches or pull requests