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

Prestwich/gas escalator dangle #2284

Merged
merged 9 commits into from Mar 21, 2023
Merged

Conversation

prestwich
Copy link
Collaborator

@prestwich prestwich commented Mar 21, 2023

Fixes #2269

Motivation

Task spawn was causing dangling tasks when GasEscalator dropped, as no messaging notified the task to close

Solution

GasEscalatorMiddleware is split into 3 things:

  • GasEscalatorMiddleware: a simple wrapper around an arc for safe cloning
  • GasEscalatorMiddlewareInternal: Holds the inner mware, the shutdown channel, and a ref to the tx vec
  • EscalatorTask: the running escalation task. Periodically attempts to escalate the transactions

What I fixed:

  • dangling tasks
  • a very long mutex guard hold
  • gas escalator now always cheaply cloneable
  • remove clone bounds from some functions

What I didn't fix:

  • if the escalation task crashes, the mware will still become unusable, but not inform you of this or error. This is not a change from previous behavior, however, it sucks. It would require a much larger fix tho

What this introduces

  • if the user drops all middleware, escalation will cease. this seems like good, expected behavior

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@prestwich prestwich self-assigned this Mar 21, 2023
@prestwich prestwich marked this pull request as ready for review March 21, 2023 01:24
Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

One idea I had is that maybe we pull SignerMiddleware to ethers-signers and we otherwise pull out ethers-middleware to a separate repo, as these are starting to be a maintenance overhead and are not part of the "core" evolving suite.

@gakonst gakonst merged commit 080bb2e into master Mar 21, 2023
13 of 15 checks passed
@gakonst gakonst deleted the prestwich/gas-escalator-dangle branch March 21, 2023 18:18
@prestwich
Copy link
Collaborator Author

One idea I had is that maybe we pull SignerMiddleware to ethers-signers and we otherwise pull out ethers-middleware to a separate repo, as these are starting to be a maintenance overhead and are not part of the "core" evolving suite.

i'd like to address this in ethers-next

also, don't merge master into branches :P

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

Successfully merging this pull request may close these issues.

Gas escalator middleware can lead to file descriptor leaks
2 participants