Rust::com Rust Stream API Implementation#480
Conversation
4bc6faf to
9bffdbe
Compare
* Proposal for async stream receive api with basic sequence diagram
* Implemented poll_next function for stream api in consumer crate
* Unpin bound required with impl stream return
* Added Test case for to_stream api usage
* Removed the max sample paramter from API * Removed the overflow parameter
9bffdbe to
595751d
Compare
* Stream taking ProxyEventManagerGuard as value * Updated field type to Option
595751d to
95cd73f
Compare
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| #[ignore = "This test demonstrates async receive with timeout, it can run individually if wanted to test timeout behavior"] |
There was a problem hiding this comment.
This test cases ignored in example app because of it required separate service instance.
We already have plan to optimize this example app which can run as an app and can select which API want to test using user input - #489
| /// | ||
| /// # Errors | ||
| /// Returns an error if a problem occurs during sample reception. | ||
| fn to_stream<'a>(&'a self) -> impl Stream<Item = Result<Self::Sample<'a>>> + Unpin + 'a; |
There was a problem hiding this comment.
This should be &mut self, so that receive (or a 2nd to_stream call) cannot happen.
There was a problem hiding this comment.
yes agreed, updated the API signature.
| offered_producer: VehicleOfferedProducer<R>, | ||
| ) -> VehicleOfferedProducer<R> { | ||
| for i in 0..10 { | ||
| for i in 0..6 { |
There was a problem hiding this comment.
Why is 6 better than 10?
There was a problem hiding this comment.
It was reduced because we have now more test and sending 10 sample is taking time because each test is invoking producer but as already i have mention in above comment , we will optimize this example app from test based to proper API function based testing using user input from CLI , ticket for same #489
| println!("[RECEIVER] Async data processor started"); | ||
| let mut buffer = SampleContainer::new(5); | ||
| for _ in 0..5 { | ||
| for _ in 0..3 { |
There was a problem hiding this comment.
Why is 3 better than 5? Consider turning these numbers into constants and explain why they have the limit they have.
There was a problem hiding this comment.
When we are testing developed API using this example app test, it takes some time to run because of that iteration is reduced but we will optimize this example app from test based to proper API function-based testing using user input from CLI , ticket for same #489
| struct SampleStream<'a, T: CommData + Debug> { | ||
| subscriber: &'a SubscriberImpl<T>, | ||
| sample_container: Option<SampleContainer<Sample<T>>>, | ||
| event_guard: Option<ProxyEventManagerGuard<'a>>, |
There was a problem hiding this comment.
Why does this need to be an Option? I do not see any place where this is set to None.
There was a problem hiding this comment.
This issue was originally caused by the borrow checker. When calling fn try_receive_samples, it required mutable access to both values. To address this, we previously wrapped them in Option, took temporary ownership, and then reassigned them.
Now, this workaround has been removed because we are borrowing self mutably only once and using it to call internal fields when passing values to the function call so only one mut borrow happening with this now.
|
|
||
| fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
| // Yield any buffered samples from a previous batch fetch first. | ||
| if let Some(mut container) = self.sample_container.take() { |
There was a problem hiding this comment.
Why do you need to take the container? Wouldn't a simple pop_front be enough? You reassign it in any case.
There was a problem hiding this comment.
it was because of Option and borrow checker issue, removed now.
|
|
||
| let samples_received = { | ||
| // Temporarily take ownership of scratch to avoid borrow issues | ||
| if let Some(mut scratch) = self.sample_container.take() { |
There was a problem hiding this comment.
Same. Why take the container? In case of an error, the stream is then broken, since the container "gets lost".
There was a problem hiding this comment.
it was because of Option and borrow checker issue, removed now.
|
|
||
| match samples_received { | ||
| Ok(_count) => { | ||
| if let Some(mut container) = self.sample_container.take() { |
There was a problem hiding this comment.
it was because of Option and borrow checker issue, removed now.
|
|
||
| #[allow(clippy::manual_async_fn)] | ||
| fn to_stream<'a>(&'a self) -> impl Stream<Item = Result<Self::Sample<'a>>> + Unpin + 'a { | ||
| stream::empty() |
There was a problem hiding this comment.
Is there a ticket to extend this at some point?
* API is taking mut self * Removed Option for internal field
| None => Poll::Pending, | ||
| } | ||
| } | ||
| Err(e) => Poll::Ready(Some(Err(e))), |
There was a problem hiding this comment.
Stream need to returns Poll::Ready(None) for dropping the stream.
Should library close the stream after error occurred or user can handle from its side to drop the stream when error occurred.
while let Some(item) = stream.next().await {
match item {
Ok(v) => {}
Err(e) => break, // stop consuming
}
}
drop(stream);
we can explicitly mention in API documentation a bout user needs to drop the stream if error occurred and implement the drop function if required in library for stream type.
library side Stream close -
We need to introduce one more flag to track the error and if error occurred , will return the error and in iteration of next call will return the None.
// Close stream immediately in next iteration after error
if self.errored {
return Poll::Ready(None);
}
There was a problem hiding this comment.
I think combination of both the approach will make better solution, we will ask user to explicitly call the drop on error but if it does not call then we can terminate when next iteration received to library by user.
#371