Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Defer IDGenerator initialization until first use #1228

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Oct 1, 2020

Initializing the IDGenerator from init() means that any downstream
project which transitively imports this package somewhere in its
dependency tree will incur getrandom() syscalls it has no control over
at startup.

This causes problems for us in
Ignition, where we're transitively
pulling in this package via cloud.google.com/go/storage. Ignition runs
very early during the boot process, which means that even though this
isn't using GRND_RANDOM, the getrandom syscall can block until the
entropy pool is ready. This is a real problem when running in VMs on
systems which don't provide a virtualized RNG device (such as VMware) or
which lack RDRAND.

I can't find a good reference for this, but I think in general it should
be considered good practice to avoid I/O like this in init() in favour
of a more lazy approach (or an explicit Initialize() function for
clients to call).

Otherwise, every program which pulls in the package will pay for it,
whether or not they intend to actually make use of the functionality
those syscalls are priming. (While it's not relevant here, another
advantage of not using init() for this is that I/O is fallible, and
init() semantics means errors can't be handled sanely.)

Let's rework things here so that we don't actually initialize the
IDGenerator fields until the first time it's used.

@jlebon jlebon requested review from rakyll, rghetia and a team as code owners October 1, 2020 03:17
@jlebon
Copy link
Contributor Author

jlebon commented Oct 1, 2020

Apologies for the drive-by contribution. For full disclosure, I only have a superficial understanding of this codebase and I'll admit that I haven't tested a trace with this patch. Unit tests do pass locally though.

Copy link

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know this codebase either, but the change here looks sane to me.

(Aside: in Rust there is no init() equivalent, people use lazy_static! instead which naturally avoids this type of problem)

@nilebox
Copy link

nilebox commented Oct 6, 2020

Thanks @jlebon for writing up the details of the issue and its impact!
Please note that the OpenCensus project is in maintenance as it's getting replaced with the OpenTelemetry project, so we are only considering changes which address critical issues / use cases, and based on the description your PR matches that criteria.

Let me carefully review this change and get back to you in the next few days.

Copy link

@nilebox nilebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended change makes sense to me, though I left a few suggestions to make it cleaner and safer.

trace/trace.go Outdated Show resolved Hide resolved
trace/trace.go Outdated Show resolved Hide resolved
trace/trace.go Outdated Show resolved Hide resolved
@jlebon jlebon force-pushed the pr/defer-rand branch 3 times, most recently from 36b544d to cb24a8c Compare October 6, 2020 21:48
Initializing the `IDGenerator` from `init()` means that any downstream
project which transitively imports this package somewhere in its
dependency tree will incur `getrandom()` syscalls it has no control over
at startup.

This causes problems for us in
[Ignition](https://github.com/coreos/ignition), where we're transitively
pulling in this package via cloud.google.com/go/storage. Ignition runs
very early during the boot process, which means that even though this
isn't using `GRND_RANDOM`, the `getrandom` syscall can block until the
entropy pool is ready. This is a real problem when running in VMs on
systems which don't provide a virtualized RNG device (such as VMware) or
which lack RDRAND.

I can't find a good reference for this, but I think in general it should
be considered good practice to avoid I/O like this in `init()` in favour
of a more lazy approach (or an explicit `Initialize()` function for
clients to call).

Otherwise, *every* program which pulls in the package will pay for it,
whether or not they intend to actually make use of the functionality
those syscalls are priming. (While it's not relevant here, another
advantage of not using `init()` for this is that I/O is fallible, and
`init()` semantics means errors can't be handled sanely.)

Let's rework things here so that we don't actually initialize the
`IDGenerator` fields until the first time it's used.
@nilebox nilebox merged commit 3fb168f into census-instrumentation:master Oct 6, 2020
@nilebox
Copy link

nilebox commented Oct 6, 2020

@jlebon you may switch to a pinned commit for dependency or wait until we make a new release for opencensus-go.

@nilebox
Copy link

nilebox commented Oct 7, 2020

jlebon added a commit to jlebon/ignition that referenced this pull request Oct 7, 2020
This includes the patch to avoid `getrandom()` calls:
census-instrumentation/opencensus-go#1228
jlebon added a commit to jlebon/ignition that referenced this pull request Oct 7, 2020
This includes the patch to avoid `getrandom()` calls:
census-instrumentation/opencensus-go#1228
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants