Skip to content

Commit

Permalink
Relax an assertion in start_selection()
Browse files Browse the repository at this point in the history
It asserted that the previous count was always nonnegative, but DISCONNECTED is
a valid value for it to see. In order to continue to remember to store
DISCONNECTED after DISCONNECTED was seen, I also added a helper method.

Closes #12226
  • Loading branch information
alexcrichton committed Feb 13, 2014
1 parent 58eeb07 commit ad69b1f
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 10 deletions.
96 changes: 94 additions & 2 deletions src/libstd/comm/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ impl Select {
/// event could either be that data is available or the corresponding
/// channel has been closed.
pub fn wait(&self) -> uint {
self.wait2(false)
}

/// Helper method for skipping the preflight checks during testing
fn wait2(&self, do_preflight_checks: bool) -> uint {
// Note that this is currently an inefficient implementation. We in
// theory have knowledge about all ports in the set ahead of time, so
// this method shouldn't really have to iterate over all of them yet
Expand All @@ -179,7 +184,7 @@ impl Select {
let mut amt = 0;
for p in self.iter() {
amt += 1;
if (*p).packet.can_recv() {
if do_preflight_checks && (*p).packet.can_recv() {
return (*p).id;
}
}
Expand Down Expand Up @@ -507,7 +512,7 @@ mod test {
let (p2, c2) = Chan::<()>::new();
let (p, c) = Chan::new();
spawn(proc() {
let mut s = Select::new();
let s = Select::new();
let mut h1 = s.handle(&p1);
let mut h2 = s.handle(&p2);
unsafe { h2.add(); }
Expand All @@ -521,4 +526,91 @@ mod test {
c2.send(());
p.recv();
})

test!(fn preflight1() {
let (p, c) = Chan::new();
c.send(());
select!(
() = p.recv() => {},
)
})

test!(fn preflight2() {
let (p, c) = Chan::new();
c.send(());
c.send(());
select!(
() = p.recv() => {},
)
})

test!(fn preflight3() {
let (p, c) = Chan::new();
drop(c.clone());
c.send(());
select!(
() = p.recv() => {},
)
})

test!(fn preflight4() {
let (p, c) = Chan::new();
c.send(());
let s = Select::new();
let mut h = s.handle(&p);
unsafe { h.add(); }
assert_eq!(s.wait2(false), h.id);
})

test!(fn preflight5() {
let (p, c) = Chan::new();
c.send(());
c.send(());
let s = Select::new();
let mut h = s.handle(&p);
unsafe { h.add(); }
assert_eq!(s.wait2(false), h.id);
})

test!(fn preflight6() {
let (p, c) = Chan::new();
drop(c.clone());
c.send(());
let s = Select::new();
let mut h = s.handle(&p);
unsafe { h.add(); }
assert_eq!(s.wait2(false), h.id);
})

test!(fn preflight7() {
let (p, c) = Chan::<()>::new();
drop(c);
let s = Select::new();
let mut h = s.handle(&p);
unsafe { h.add(); }
assert_eq!(s.wait2(false), h.id);
})

test!(fn preflight8() {
let (p, c) = Chan::new();
c.send(());
drop(c);
p.recv();
let s = Select::new();
let mut h = s.handle(&p);
unsafe { h.add(); }
assert_eq!(s.wait2(false), h.id);
})

test!(fn preflight9() {
let (p, c) = Chan::new();
drop(c.clone());
c.send(());
drop(c);
p.recv();
let s = Select::new();
let mut h = s.handle(&p);
unsafe { h.add(); }
assert_eq!(s.wait2(false), h.id);
})
}
18 changes: 14 additions & 4 deletions src/libstd/comm/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,17 @@ impl<T: Send> Packet<T> {
cnt == DISCONNECTED || cnt - self.steals > 0
}

// increment the count on the channel (used for selection)
fn bump(&mut self, amt: int) -> int {
match self.cnt.fetch_add(amt, atomics::SeqCst) {
DISCONNECTED => {
self.cnt.store(DISCONNECTED, atomics::SeqCst);
DISCONNECTED
}
n => n
}
}

// Inserts the blocked task for selection on this port, returning it back if
// the port already has data on it.
//
Expand All @@ -408,8 +419,8 @@ impl<T: Send> Packet<T> {
match self.decrement(task) {
Ok(()) => Ok(()),
Err(task) => {
let prev = self.cnt.fetch_add(1, atomics::SeqCst);
assert!(prev >= 0);
let prev = self.bump(1);
assert!(prev == DISCONNECTED || prev >= 0);
return Err(task);
}
}
Expand Down Expand Up @@ -440,11 +451,10 @@ impl<T: Send> Packet<T> {
let cnt = self.cnt.load(atomics::SeqCst);
if cnt < 0 && cnt != DISCONNECTED {-cnt} else {0}
};
let prev = self.cnt.fetch_add(steals + 1, atomics::SeqCst);
let prev = self.bump(steals + 1);

if prev == DISCONNECTED {
assert_eq!(self.to_wake.load(atomics::SeqCst), 0);
self.cnt.store(DISCONNECTED, atomics::SeqCst);
true
} else {
let cur = prev + steals + 1;
Expand Down
18 changes: 14 additions & 4 deletions src/libstd/comm/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,17 @@ impl<T: Send> Packet<T> {
}
}

// increment the count on the channel (used for selection)
fn bump(&mut self, amt: int) -> int {
match self.cnt.fetch_add(amt, atomics::SeqCst) {
DISCONNECTED => {
self.cnt.store(DISCONNECTED, atomics::SeqCst);
DISCONNECTED
}
n => n
}
}

// Attempts to start selecting on this port. Like a oneshot, this can fail
// immediately because of an upgrade.
pub fn start_selection(&mut self, task: BlockedTask) -> SelectionResult<T> {
Expand All @@ -351,8 +362,8 @@ impl<T: Send> Packet<T> {
};
// Undo our decrement above, and we should be guaranteed that the
// previous value is positive because we're not going to sleep
let prev = self.cnt.fetch_add(1, atomics::SeqCst);
assert!(prev >= 0);
let prev = self.bump(1);
assert!(prev == DISCONNECTED || prev >= 0);
return ret;
}
}
Expand Down Expand Up @@ -384,13 +395,12 @@ impl<T: Send> Packet<T> {
// and in the stream case we can have at most one steal, so just assume
// that we had one steal.
let steals = 1;
let prev = self.cnt.fetch_add(steals + 1, atomics::SeqCst);
let prev = self.bump(steals + 1);

// If we were previously disconnected, then we know for sure that there
// is no task in to_wake, so just keep going
let has_data = if prev == DISCONNECTED {
assert_eq!(self.to_wake.load(atomics::SeqCst), 0);
self.cnt.store(DISCONNECTED, atomics::SeqCst);
true // there is data, that data is that we're disconnected
} else {
let cur = prev + steals + 1;
Expand Down

4 comments on commit ad69b1f

@bors
Copy link
Contributor

@bors bors commented on ad69b1f Feb 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from kballard
at alexcrichton@ad69b1f

@bors
Copy link
Contributor

@bors bors commented on ad69b1f Feb 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging alexcrichton/rust/chan-select-bug = ad69b1f into auto

@bors
Copy link
Contributor

@bors bors commented on ad69b1f Feb 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alexcrichton/rust/chan-select-bug = ad69b1f merged ok, testing candidate = 881741e8

@bors
Copy link
Contributor

@bors bors commented on ad69b1f Feb 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.