chore(tests): Adjust buffering tests #2862
Conversation
Signed-off-by: Kruno Tomola Fabro <[email protected]>
Signed-off-by: Kruno Tomola Fabro <[email protected]>
tests/buffering.rs
Outdated
| // Give the topology some time to process the received data and simulate | ||
| // a crash. | ||
| // We need to wait for at least the source to process events. | ||
| wait_for(|| source_event_counter.load(Ordering::Acquire) == num_events); |
There was a problem hiding this comment.
So, this means that the changes from e0761a9 don't fix the issue :(
Can we restore the comment that was there before the said commit? I think it's more on-point, and gives hints on where the root cause of the issue is. Unless it's not relevant anymore.
There was a problem hiding this comment.
It fixed races relevant to the PR, but there is another race in MockSourceConfig that we can't avoid without changing the channel impl to some that doesn't use buffering even then there would be a race in the source, but even then just source processing events isn't enough, they still need to reach the buffers of source and yet still they need to reach file to which we are buffering those events. Only some intrusive synchronizing could fix this.
Because of the first race, the assert isn't appropriate.
There was a problem hiding this comment.
Yeah! The code before e0761a9 was:
// A race caused by `rt.block_on(send).unwrap()` is handled here. For some
// reason, at times less events than were sent actually arrive to the
// `source`.
// We mitigate that by waiting on the event counter provided by our source
// mock.
test_util::wait_for_atomic_usize(source_event_counter, |x| x == num_events);It's pretty much what you said, so I suggest just restoring it as it was.
The assertion can be the same as you have in this PR, I'm mostly worried about the comment.
This isn't critical though.
There was a problem hiding this comment.
So, to improve the overall situation, we have to reimplement the MockSourceConfig, right?
There was a problem hiding this comment.
Ok, I'll document the race for which we need to wait. And use that test_util::wait_for_atomic_usize.
So, to improve the overall situation, we have to reimplement the MockSourceConfig, right?
I don't think its possible to get better than waiting for source_event_counter.
Signed-off-by: Kruno Tomola Fabro <[email protected]>
Signed-off-by: Kruno Tomola Fabro <[email protected]>
|
New error started occurring, I think it's a bug that existed before this PR so I'll be merging this in spite of it. |
* Fix buffering tests Signed-off-by: Kruno Tomola Fabro <[email protected]> * Split wait Signed-off-by: Kruno Tomola Fabro <[email protected]> * Document the race Signed-off-by: Kruno Tomola Fabro <[email protected]> * Remove unneded Signed-off-by: Kruno Tomola Fabro <[email protected]> Signed-off-by: Brian Menges <[email protected]>
This PR does three things:
bufferingtests.thread::sleepinbufferingtests, reducing waiting in total.crate::test_util::CountReceiver::waitmethod intowaitandcancel. All of the usages of this method were having a race as they were expecting that the method would wait for thetcpchannel to close, which wasn't the case. Instead the method was canceling the channel which caused a race.