1
Fork 0

auto merge of #12302 : alexcrichton/rust/issue-12295, r=brson

The previous code erroneously assumed that 'steals > cnt' was always true, but
that was a false assumption. The code was altered to decrement steals to a
minimum of 0 instead of taking all of cnt into account.

I didn't include the exact test from #12295 because it could run for quite
awhile, and instead set the threshold for MAX_STEALS to much lower during
testing. I found that this triggered the old bug quite frequently when running
without this fix.

Closes #12295
This commit is contained in:
bors 2014-02-15 23:36:26 -08:00
commit b36340b626
5 changed files with 28 additions and 10 deletions

View file

@ -18,6 +18,7 @@
/// module. You'll also note that the implementation of the shared and stream /// module. You'll also note that the implementation of the shared and stream
/// channels are quite similar, and this is no coincidence! /// channels are quite similar, and this is no coincidence!
use cmp;
use int; use int;
use iter::Iterator; use iter::Iterator;
use kinds::Send; use kinds::Send;
@ -35,6 +36,9 @@ use mpsc = sync::mpsc_queue;
static DISCONNECTED: int = int::MIN; static DISCONNECTED: int = int::MIN;
static FUDGE: int = 1024; static FUDGE: int = 1024;
#[cfg(test)]
static MAX_STEALS: int = 5;
#[cfg(not(test))]
static MAX_STEALS: int = 1 << 20; static MAX_STEALS: int = 1 << 20;
pub struct Packet<T> { pub struct Packet<T> {
@ -307,7 +311,11 @@ impl<T: Send> Packet<T> {
DISCONNECTED => { DISCONNECTED => {
self.cnt.store(DISCONNECTED, atomics::SeqCst); self.cnt.store(DISCONNECTED, atomics::SeqCst);
} }
n => { self.steals -= n; } n => {
let m = cmp::min(n, self.steals);
self.steals -= m;
self.cnt.fetch_add(n - m, atomics::SeqCst);
}
} }
assert!(self.steals >= 0); assert!(self.steals >= 0);
} }

View file

@ -17,6 +17,7 @@
/// High level implementation details can be found in the comment of the parent /// High level implementation details can be found in the comment of the parent
/// module. /// module.
use cmp;
use comm::Port; use comm::Port;
use int; use int;
use iter::Iterator; use iter::Iterator;
@ -32,6 +33,9 @@ use sync::atomics;
use vec::OwnedVector; use vec::OwnedVector;
static DISCONNECTED: int = int::MIN; static DISCONNECTED: int = int::MIN;
#[cfg(test)]
static MAX_STEALS: int = 5;
#[cfg(not(test))]
static MAX_STEALS: int = 1 << 20; static MAX_STEALS: int = 1 << 20;
pub struct Packet<T> { pub struct Packet<T> {
@ -198,11 +202,16 @@ impl<T: Send> Packet<T> {
pub fn try_recv(&mut self) -> Result<T, Failure<T>> { pub fn try_recv(&mut self) -> Result<T, Failure<T>> {
match self.queue.pop() { match self.queue.pop() {
// If we stole some data, record to that effect (this will be // If we stole some data, record to that effect (this will be
// factored into cnt later on). Note that we don't allow steals to // factored into cnt later on).
// grow without bound in order to prevent eventual overflow of //
// either steals or cnt as an overflow would have catastrophic // Note that we don't allow steals to grow without bound in order to
// results. Also note that we don't unconditionally set steals to 0 // prevent eventual overflow of either steals or cnt as an overflow
// because it can be true that steals > cnt. // would have catastrophic results. Sometimes, steals > cnt, but
// other times cnt > steals, so we don't know the relation between
// steals and cnt. This code path is executed only rarely, so we do
// a pretty slow operation, of swapping 0 into cnt, taking steals
// down as much as possible (without going negative), and then
// adding back in whatever we couldn't factor into steals.
Some(data) => { Some(data) => {
self.steals += 1; self.steals += 1;
if self.steals > MAX_STEALS { if self.steals > MAX_STEALS {
@ -210,7 +219,11 @@ impl<T: Send> Packet<T> {
DISCONNECTED => { DISCONNECTED => {
self.cnt.store(DISCONNECTED, atomics::SeqCst); self.cnt.store(DISCONNECTED, atomics::SeqCst);
} }
n => { self.steals -= n; } n => {
let m = cmp::min(n, self.steals);
self.steals -= m;
self.cnt.fetch_add(n - m, atomics::SeqCst);
}
} }
assert!(self.steals >= 0); assert!(self.steals >= 0);
} }

View file

@ -867,7 +867,6 @@ impl num::FromStrRadix for f32 {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use f32::*; use f32::*;
use prelude::*;
use num::*; use num::*;
use num; use num;

View file

@ -869,7 +869,6 @@ impl num::FromStrRadix for f64 {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use f64::*; use f64::*;
use prelude::*;
use num::*; use num::*;
use num; use num;

View file

@ -65,7 +65,6 @@ use rt::task::Task;
use str::{Str, SendStr, IntoMaybeOwned}; use str::{Str, SendStr, IntoMaybeOwned};
#[cfg(test)] use any::{AnyOwnExt, AnyRefExt}; #[cfg(test)] use any::{AnyOwnExt, AnyRefExt};
#[cfg(test)] use ptr;
#[cfg(test)] use result; #[cfg(test)] use result;
/// Indicates the manner in which a task exited. /// Indicates the manner in which a task exited.