Gas escalator middleware can lead to file descriptor leaks

[gyuho]

Version
List the versions of all ethers-rs crates you are using. The easiest way to get
this information is using cargo-tree.

2.0.0

Platform
The output of uname -a (UNIX), or version and 32 or 64-bit (Windows)

Linux ip-10-0-67-197 5.15.0-1030-aws #34~20.04.1-Ubuntu SMP Tue Jan 24 15:16:46 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Description
Enter your issue details here.
One way to structure the description:

I am creating the provider with the gas escalator dynamically, to dynamically connect to chain endpoints. For every gas escalator middleware creation GasEscalatorMiddleware::new, it spawns an asynchronous task to poll the gas price:

ethers-rs/ethers-middleware/src/gas_escalator/mod.rs

Lines 135 to 156 in 304fe09

	#[allow(clippy::let_and_return)]
	#[cfg(not(target_arch = "wasm32"))]
	pub fn new(inner: M, escalator: E, frequency: Frequency) -> Self
	where
	E: Clone + 'static,
	M: Clone + 'static,
	{
	use tracing_futures::Instrument;
	let this = Self {
	inner: Arc::new(inner),
	escalator,
	frequency,
	txs: Arc::new(Mutex::new(Vec::new())),
	};
	{
	let this2 = this.clone();
	spawn(async move {
	this2.escalate().instrument(tracing::trace_span!("gas-escalation")).await.unwrap();
	});
	}

After some hours, we found that this likely caused the file descriptor leaks when we create GasEscalatorMiddleware for each request (even if we are done processing the request, the asynchronous task is still running).

One way is for us to reuse gas escalator, instead of creating a new one every time. Or ethers-rs exposes a way to shut down this asynchronous task.

Would appreciate any feedback. Thanks!

[mattsse]

tbh I'm unfamiliar how this is works in detail, but this

https://github.com/gakonst/ethers-rs/blob/304fe0988d23302694a63da47ce683af19305357/ethers-middleware/src/gas_escalator/mod.rs#LL153-L155C16

creates a dangling task when the Middleware is dropped which appears to be the issue you're facing.

@prestwich I think the task should resolve when the Middleware instance is dropped, also seems like a pitfall that you can escalate multiple times.

suggestion:
on new the middleware gets a Arc<oneshot::Sender<()>
and the task selects over the receiver half.

[prestwich]

we create GasEscalatorMiddleware for each request

This is not advisable, why?

I think the task should resolve when the Middleware instance is dropped, also seems like a pitfall that you can escalate multiple times.

suggestion:
on new the middleware gets a Arc<oneshot::Sender<()>
and the task selects over the receiver half.

this doesn't precisely work, as the middleware may be cloned. I'll work on splitting this up and making gas escalator clone and drop safe

[prestwich]

I think the task should resolve when the Middleware instance is dropped, also seems like a pitfall that you can escalate multiple times.

this may also break the user's usage, as if they're making 1 escalator per tx (which is not recommended), then the escalation will stop when they discard the middleware

[prestwich]

oh a further problem, if we go the way it's currently written, with an Arced middleware, the middleware will NEVER drop as the task holds an arc to it 😅

[prestwich]

@gyuho I've pushed a PR to address the dangling task. Please be advised that making middleware and then dropping it is not recommended and is _not a supported use of the middleware. If you are dropping the middleware after usage, this PR will break your code in a different way. We strongly recommend cloning the middleware instead of making a new one

[gyuho]

@prestwich Appreciate the feedback. And yes, it was a bad practice on my end without knowing there was an async task for each escalator creation. So, we switched back to reusing or sharing the escalator which solves the issue.

We strongly recommend cloning the middleware instead of making a new one

Yes, this is what we ended up doing :)