This repository has been archived by the owner on Oct 19, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prestwich/super pending #566
Prestwich/super pending #566
Changes from 10 commits
e3a3713
8dd1a18
533b59d
0e3318a
29eb74c
44fb4ce
a832b48
9d6be17
d970471
1afd9c9
ed25721
7b91c26
096ca41
a001551
67a9e15
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use milliseconds here, just for flexbility sake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to change it to use
Into<Duration>
to match the behavior ofPendingTransaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this
cx.waker().wake_by_ref()
necessary? Won't executor automatically put the EscalatingPending future back in queue? Other paths here that return Poll::Pending don't call wakeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the goal is to explicitly notify the executor that this is ready to be polled again immediately. i.e. override the executor's scheduling and jump to the front of the queue as much as we have the ability to, as we know that there's no async operation happening to wait on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section looks like it could be wrapped in a loop over all futures? So that
Poll::Ready(Ok(None)) =>
merely results in a continue?Is the order in which they should be polled here important, if not then
Vec::swap_remove
could be useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean it kinda is right now, it's just ensuring that it allows the executor to schedule each step of the loop and interleave other work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looping over all futures is an anti-pattern I think, the state machine may be slightly easier to write but as @prestwich says it's better to do it on a per-fut basis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the
CheckingReceipts
state essentially aFuturesOrdered
but we only poll one future at a time?