Uh oh!
There was an error while loading. Please reload this page.
Drop futures dependency from lightning-block-sync - #2141
Conversation
TheBlueMatt
commented
Mar 30, 2023
Note, as an alternative, |
TheBlueMatt
commented
Mar 30, 2023
As a further alternative, we could use sync mutexes with an |
Some how I'd understood that `futures` had reasonable MSRV guarantees (e.g. at least Debian stable), but apparently that isn't actually the case, as they bumped it to upgrade to syn (with apparently no actual features or bugfixes added as a result?) with no minor version bump or any available alternative (unlike Tokio, which does LTS releases). Luckily its relatively easy to just drop the `futures` dependency - it means a new connection for each request, which is annoying, but certainly not the end of the world, and its easier than trying to deal with pinning `futures`. See rust-lang/futures-rs#2733
8699320 to
78c73b0CompareTheBlueMatt
commented
Mar 30, 2023
Actually, went ahead and did the second because its super trivial. |
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMatt
commented
Mar 30, 2023
Grr, also need to replace the futures usages in the async BP. I think its quite doable though. |
a041fc9 to
cf0e7dcCompareTheBlueMatt
commented
Mar 30, 2023
Ok, removed it from BP wholesale in two commits. Honestly dunno why we were relying on a whole mess of proc macro in the futures crate to implement select lol. |
In general, only one request will be in flight at a time in `lightning-block-sync`. Ideally we'd only have one connection, but without using the `futures` mutex type. Here we solve this narrowly for the one-request-at-a-time case by caching the connection and takeing the connection out of the cache while we work on it.
cf0e7dc to
152ac15CompareTheBlueMatt
commented
Mar 30, 2023
That said, I'm open to pushback on the last commit (or the second one) - it does add a trivial use of unsafe, which is trivial, but technically its not required to fix MSRV, it just leaves us open to futures breaking us again in the future. |
Uh oh!
There was an error while loading. Please reload this page.
| const DUMMY_WAKER_VTABLE: RawWakerVTable = RawWakerVTable::new( | ||
| dummy_waker_clone, dummy_waker_action, dummy_waker_action, dummy_waker_action); | ||
| pub(crate) fn dummy_waker() -> Waker { unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &DUMMY_WAKER_VTABLE)) } } |
There was a problem hiding this comment.
what would not introducing this unsafe look like?
There was a problem hiding this comment.
Using another crate which has the unsafe in it instead. There's no way to poll a future in rust with zero unsafe. Now, we could consider coming up with some other way to do no-std timers inside an async context outside of polling a timer future, but I'm not 100% sure what is cleaner for users.
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@## main #2141 +/- ##
==========================================
- Coverage 91.38% 91.37% -0.02%
==========================================
Files 102 102 Lines 49759 49767 +8 Branches 49759 49767 +8 ==========================================
+ Hits 45472 45474 +2 - Misses 4287 4293 +6
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
`futures` recently broke our MSRV by bumping the `syn` major version in a patch release. This makes it impractical for us to use, instead here we replace the usage of its `select_biased` macro with a trivial enum. Given its simplicity we likely should have done this without ever taking the dependency.
152ac15 to
718ab54CompareAs `futures` apparently makes no guarantees on MSRVs even in patch releases we really can't rely on it at all, and while it currently has an acceptable MSRV without the macros feature, its best to just remove it wholesale. Luckily, removing it is relatively trivial, even if it requires the most trivial of unsafe tags.
718ab54 to
491100dCompareTheBlueMatt
commented
Mar 31, 2023
Addressed arik's comments and squashed. |
| // not without awaiting, we need a Waker, which needs a vtable...we fill it with dummy values | ||
| // but sadly there's a good bit of boilerplate here. | ||
| fn dummy_waker_clone(_: *const ()) -> RawWaker { RawWaker::new(core::ptr::null(), &DUMMY_WAKER_VTABLE) } | ||
| fn dummy_waker_action(_: *const ()) { } |
There was a problem hiding this comment.
given these are dummies, would it make sense to insert some unreachable!s?
There was a problem hiding this comment.
Not quite, they're still called, but they're no-op's.
Some how I'd understood that
futureshad reasonable MSRV guarantees (e.g. at least Debian stable), but apparently that isn't actually the case, as they bumped it to upgrade to syn (with apparently no actual features or bugfixes added as a result?) with no minor version bump or any available alternative (unlike Tokio, which does LTS releases).Luckily its relatively easy to just drop the
futuresdependency - it means a new connection for each request, which is annoying, but certainly not the end of the world, and its easier than trying to deal with pinningfutures.See rust-lang/futures-rs#2733