chore: fix setting tracing subscriber

[agostbiro]

Previously we were only setting a default tracing subscriber for the current thread in edr_napi which precluded logs from other threads being collected.

With this change, the subscriber is set globally for all threads so that we can get tracing output when running JS tests, e.g. from hardhat-tests: RUST_LOG=debug pnpm test.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 0c13cda

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Wodann]

The documentation mentions:

Note: Libraries should NOT call set_global_default()! That will cause conflicts when executables try to set them later.

It seems like we might want to use with_default inside each function to prevent causing crashes if other shared libraries are also using tracing.

We have a similar pending task for fixing the tokio runtime. That's currently blocked by the reqwest_middleware not allowing a custom tokio runtime to be set.

WDYT?

[agostbiro]

Hey, thanks for the review!

It's a good point that if another Rust lib imported in NodeJS sets set_global_default() that could cause problems. It wouldn't cause a panic though (at least not in EDR), because in this PR we return a NAPI error from the EdrContext constructor if the global subscriber is already set. Still this is not ideal, because it means that if the user imports some other library that also sets the global subscriber, it can prevent Hardhat from being initialized.

I don't think switching to with_default is feasible, because it only works for the current thread and there are a lot of places where new threads are spawned explicitly or implicitly and it'd be impossible to ensure that we cover all those cases. (RUST_LOG is not just useful to see our own traces, but we can do e.g. RUST_LOG="tokio=debug" to get debug traces from tokio).

So I see two possible solutions:

1. The best one is imo if we factor out the logger initialization from EdrContext::new into a standalone function that Hardhat should call when it sees fit, but only once. The function can throw if the global subscriber is already set, but it doesn't prevent using EDR and Hardhat can handle the error as it sees fit.
2. The quick fix that doesn't need changes in Hardhat would be to not propagate the error from set_global_default() here, but instead just print a warning and then ignore it.

Resolving this would be quite important, because it's a valuable debugging tool to observe released binaries.

[agostbiro]

(Btw I had a quick look and our version of tracing doesn't seem to set no_mangle, so maybe other NodeJS shared Rust lib calling it is not an issue?)

[Wodann]

(Btw I had a quick look and our version of tracing doesn't seem to set no_mangle, so maybe other NodeJS shared Rust lib calling it is not an issue?)

If mangle is used in the same way as it normally is (i.e. mangle a function name to be unique), I'm not sure how it would prevent the issue. The static memory location of the default logger would still be reused, right?

[Wodann]

The safest bet might be to log a warning when we can't set a default logger? Then we can at least get debug logs to work for now, without causing crashes.

We can find a permanent solution in a follow-up task.

[agostbiro]

If mangle is used in the same way as it normally is (i.e. mangle a function name to be unique), I'm not sure how it would prevent the issue. The static memory location of the default logger would still be reused, right?

Right, mangling is only relevant if the other shared lib was a C lib, but another Rust shared lib will have the same mangled name and can use that to look up the address of the static that holds the global logger? I'm unsure of the details of how this works exactly, but I'm curious to learn more.

[agostbiro]

The safest bet might be to log a warning when we can't set a default logger? Then we can at least get debug logs to work for now, without causing crashes.

Ok, done in 0f405d2