-
Notifications
You must be signed in to change notification settings - Fork 249
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
refactor(ipc
): use single buffer and remove manual wakers
#69
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,13 +23,12 @@ pub mod mock; | |
#[cfg(feature = "mock")] | ||
pub use mock::MockIpcServer; | ||
|
||
use std::task::Poll::{Pending, Ready}; | ||
|
||
use alloy_json_rpc::PubSubItem; | ||
use bytes::{Buf, BytesMut}; | ||
use futures::{io::BufReader, ready, AsyncBufRead, AsyncRead, AsyncWriteExt, StreamExt}; | ||
use futures::{ready, AsyncRead, AsyncWriteExt, StreamExt}; | ||
use interprocess::local_socket::{tokio::LocalSocketStream, ToLocalSocketName}; | ||
use std::task::Poll::Ready; | ||
use tokio::select; | ||
use tokio_util::compat::FuturesAsyncReadCompatExt; | ||
|
||
type Result<T> = std::result::Result<T, std::io::Error>; | ||
|
||
|
@@ -113,19 +112,17 @@ impl IpcBackend { | |
pub struct ReadJsonStream<T> { | ||
/// The underlying reader. | ||
#[pin] | ||
reader: BufReader<T>, | ||
/// A buffer of bytes read from the reader. | ||
reader: tokio_util::compat::Compat<T>, | ||
/// A buffer for reading data from the reader. | ||
buf: BytesMut, | ||
/// A buffer of items deserialized from the reader. | ||
items: Vec<PubSubItem>, | ||
} | ||
|
||
impl<T> ReadJsonStream<T> | ||
where | ||
T: AsyncRead, | ||
{ | ||
fn new(reader: T) -> Self { | ||
Self { reader: BufReader::new(reader), buf: BytesMut::with_capacity(4096), items: vec![] } | ||
Self { reader: reader.compat(), buf: BytesMut::with_capacity(4096) } | ||
} | ||
} | ||
|
||
|
@@ -148,19 +145,23 @@ where | |
self: std::pin::Pin<&mut Self>, | ||
cx: &mut std::task::Context<'_>, | ||
) -> std::task::Poll<Option<Self::Item>> { | ||
let this = self.project(); | ||
use tokio_util::io::poll_read_buf; | ||
|
||
// Deserialize any buffered items. | ||
if !this.buf.is_empty() { | ||
this.reader.consume(this.buf.len()); | ||
let mut this = self.project(); | ||
|
||
loop { | ||
// try decoding from the buffer | ||
tracing::debug!(buf_len = this.buf.len(), "Deserializing buffered IPC data"); | ||
let mut de = serde_json::Deserializer::from_slice(this.buf.as_ref()).into_iter(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instantiating the deserializer performs a new alloc every time the poll is invoked. instead, let's check if the buffer is empty before instantiating, and then after instantiating, drain the whole buffer into an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a toggle |
||
|
||
let item = de.next(); | ||
|
||
// advance the buffer | ||
this.buf.advance(de.byte_offset()); | ||
|
||
match item { | ||
Some(Ok(response)) => { | ||
this.items.push(response); | ||
return Ready(Some(response)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wont this now fail to wake up again until the socket has more data? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the thing here is that you assume the read has EXACTLY ONE item in it. but that may not be the case if geth has sent multiple notifications There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh then i guess the followup would be that the underlying stream re-wakes the task as there's more data? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this yields a new value, the caller is supposed to call poll_next again, streams should only register the waker if they are pending https://docs.rs/futures/latest/futures/stream/trait.Stream.html#tymethod.poll_next
the way this loop works is: it drains the buffer by yielding deserialized values (advancing the buffer) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this doesn't mesh with my understanding of my understanding of wakers. AIUI the waker exists to notify the executor that there is additional work (i.e. that something that made us pending has become unblocked), and the executor may never poll a task that does not have a call to when they say "register for waking" I'm pretty sure what they mean is "store a copy of the waker in the local state. this is how, e.g., the as a side note, your code may work in prod because tokio has an optimization where it polls futures again immediately after they return ready (even if the waker is not triggered), in case there's more work. While it may work, I'm not sure it is correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
the executor deals with the top-level task which contains the stream state machine, and any futures or other streams or other structs that contain that stream, and schedules that task for execution. futures and streams use the wakers to communicate to the executor that the task should be scheduled.
this is a busy loop if the stream is always ready, yes. but as soon as the stream returns pending and the executor switches tasks, there is no guarantee that the executor returns to task before the task's Waker is used to re-wake it.
this is "registering" the waker by adding it to task state to be re-woken when the channel has more items, ensuring that the executor calls poll when items are available, as i described above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
right, that's why it isn't necessary to call it when returning an item There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is necessary if our buffer isn't empty, as we have work to do immediately, which wont get done if the task isnt rewoken There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the new poll loop has 4 exits:
I fail to see where we'd need to request another call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we read every item as soon as it's written, then we don't have a problem however, if the other end of the socket ever writes 2 items between polls of this stream, thne then next time the stream is polled it will fill the buffer (getting the 2 items), and then deser and return an item. At that point, we have a serialized item in the buffer. If we do not re-wake properly, then the next poll will occur when the asyncread is ready to give us more serialized items for our buffer. That wake will again result in only 1 item being read from the buffer we need to re-wake until the buffer is empty, otherwise we risk falling significantly behind and filling our buffer |
||
} | ||
Some(Err(e)) => { | ||
tracing::error!(%e, "IPC response contained invalid JSON. Buffer contents will be logged at trace level"); | ||
|
@@ -171,34 +172,20 @@ where | |
|
||
return Ready(None); | ||
} | ||
None => {} | ||
None => { | ||
// nothing decoded | ||
} | ||
} | ||
this.buf.advance(de.byte_offset()); | ||
cx.waker().wake_by_ref(); | ||
return Pending; | ||
} | ||
|
||
// Return any buffered items, rewaking. | ||
if !this.items.is_empty() { | ||
// may have more work! | ||
cx.waker().wake_by_ref(); | ||
return Ready(this.items.pop()); | ||
} | ||
|
||
tracing::debug!(buf_len = this.buf.len(), "Polling IPC socket for data"); | ||
|
||
let data = ready!(this.reader.poll_fill_buf(cx)); | ||
match data { | ||
Err(e) => { | ||
tracing::error!(%e, "Failed to read from IPC socket, shutting down"); | ||
Ready(None) | ||
} | ||
Ok(data) => { | ||
tracing::debug!(data_len = data.len(), "Read data from IPC socket"); | ||
this.buf.extend_from_slice(data); | ||
// wake task to run deserialization | ||
cx.waker().wake_by_ref(); | ||
Pending | ||
// read more data into the buffer | ||
match ready!(poll_read_buf(this.reader.as_mut(), cx, &mut this.buf)) { | ||
Ok(data_len) => { | ||
tracing::debug!(%data_len, "Read data from IPC socket"); | ||
} | ||
Err(e) => { | ||
tracing::error!(%e, "Failed to read from IPC socket, shutting down"); | ||
return Ready(None); | ||
} | ||
} | ||
} | ||
} | ||
|
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.
TIL, although, what a name lol. I wasted some time looking for something like this 😅
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 was looking for that! i found the old 0.3 compat crate but couldn't find the new one
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 previously wasted a few hours on this myself -.-
same with finding a properly maintained
tokio::codec
Json
implementation.using SerializeStream is an adequate solution I think