Make sure ShardSenders are dropped before ShardWriter

[sreenathkrishnan]

Changes

1. Include an immutable reference to theShardWriter inside ShardSender
2. ShardSender<T> is now ShardSender<'a, T, S>
3. This required adding the trait bound S: Send, but that should not be restrictive at all
4. Update the doc example

Illegal code

API usage of the form:

{
    let mut writer: ShardWriter<...> = ShardWriter::new(...);
    let mut sender = writer.get_sender();
    ...
    writer.finish()?;
}

will not compile with a clear message

error[E0502]: cannot borrow `writer` as mutable because it is also borrowed as immutable
  --> src/lib.rs:45:9
   |
31 |         let mut sender = writer.get_sender();
   |                          ------ immutable borrow occurs here
...
39 |         writer.finish()?;
   |         ^^^^^^^^^^^^^^^ mutable borrow occurs here
40 |     }
   |     - immutable borrow might be used here, when `sender` is dropped and runs the `Drop` code for type `shardio::ShardSender`

Legal code

You can either not call finish():

{
    let writer: ShardWriter<...> = ShardWriter::new(...);
    let mut sender = writer.get_sender();
    ...
}

or explicitly scope the sender and call finish

{
    let mut writer: ShardWriter<...> = ShardWriter::new(...);
    {
        let mut sender = writer.get_sender();
        ...
    }
    writer.finish()
}

[coveralls]

Pull Request Test Coverage Report for Build 59

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 36 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.005%) to 95.233%

---

Files with Coverage Reduction	New Missed Lines	%
lib.rs	36	94.88%

Totals
Change from base Build 57:	0.005%
Covered Lines:	879
Relevant Lines:	923

---

💛 - Coveralls

[pmarks]

Interesting! I think this means we can't send the ShardSender to a new thread, because there's no way to ensure that a thread finishes with a static lifetime. scoped threads are an option: https://docs.rs/crossbeam/0.6.0/crossbeam/thread/index.html
but I'm not sure they're sufficiently general. I need to think a bit about whether we care about the multi-threaded case

[nlhepler]

We could just mediate that with an Arc instead of a bare reference. The get_sender convenience API may need to go away though..

[nlhepler]

Actually, we could implement a trait to carry get_sender and implement it for Arc.

[pmarks]

I think Sreenath's idea is to force the lifetime of the sender to be less than the lifetime of the writer, so that when you drop / call finish() on the writer, you're guaranteed to be done sending. If we put the writer in Arc<> then I think we have the same problem as now: you can close the writer before the senders.

(As an aside, we should see if we can do impl ShardWriter { fn finish(self) -> Result<> } so that finish() consume the writer.

[pmarks]

@nlhepler - sorry - if the sender is inside Arc<>, then you can't call finish() on it. You'd need something like 'join' that could wait until all the other handles are gone, then gives you back exclusive ownership. That could deadlock on a single thread if you don't drop the senders before you 'join'.

[sreenathkrishnan]

Yeah, I don't see a way to work around both lifetime order and 'static requirement using Arc. Scoped threads seem to be the right primitive to use if you want multi-threaded support for senders. This works as expected:

let writer = ...
crossbeam_utils::thread::scope(|scope| {
    let mut sender = writer.get_sender();
    scope.spawn(move |_| {
        for i in 0..(2 << 16) {
            sender.send(DataStruct { a: (i%25) as u64, b: (i%100) as u32 });
        }
    });
}).unwrap();

Is this reasonable?

[sjackman]

Please add an end-of-line character to this file. Best to configure your text editor to ensure that text files end with an end-of-line character.