vectordotdev / vrl

Vector Remap Language
Mozilla Public License 2.0
115 stars 52 forks source link

fix(stdlib): properly detect tokio runtime in `dns_lookup` #882

Closed esensar closed 2 weeks ago

esensar commented 4 weeks ago

When testing dns_lookup in vector in a real environment, we got this error:

thread 'vector-worker' panicked at
/cargo/registry/src/index.crates.io-6f17d22bba15001f/domain-0.10.0/src/resolv/stub/mod.rs:287:17:
Cannot start a runtime from within a runtime. This happens because a
function (like `block_on`) attempted to block the current thread while
the thread is being used to drive asynchronous tasks.
note: run with `RUST_BACKTRACE=1` environment variable to display a
backtrace

Which happens because VRL is called in a tokio managed thread. The solution here was to move the call to another thread, because internally domain crate implement blocking calls by still relyting on async function implementation.

Let me know if you have a better solution for this, since this might be too much to spawn a thread for each lookup.

pront commented 4 weeks ago

Hi @esensar, thanks for suggesting a fix.

since this might be too much to spawn a thread for each lookup.

This might be problematic, could we have a pool or a dedicated thread for this?

jszwedko commented 3 weeks ago

@esensar just a heads up that we'll be releasing Vector next week in-case you want to try to fix this before then.

esensar commented 3 weeks ago

I have implemented just a single dedicated thread for this. I know that is probably not the best idea, but this close to the release, I didn't want to risk breaking stuff by adding a thread pool (which would probably include a crate, since it would be better to rely on something existing already).

jszwedko commented 2 weeks ago

I'd like to get @pront's review again too.

pront commented 2 weeks ago

I'd like to get @pront's review again too.

This is significantly improved, thanks @esensar. I had a question about the channel capacity which I believe is important to resolve before we merge this.