Thanks for the review.
Does this work with the heartbeat packet?
Line 657-658 check for the heartbeat sequence number before we set lastAckedSeqno in ResponseProcessor.run(), so it should work as before.
dataQueue.wait(1000); is it possible to use dataQueue.wait();
Yes, I think that would also work. In theory, the timeout isn't necessary, but I've seen bugs before where a slight race causes us to miss a notify. Perhaps we can switch the synchronized (dataQueue) and the while (!closed) in this function and avoid the race? It seems to me that waking up once a second just to be extra safe doesn't really harm us, but maybe it's a bit of a band-aid.
lines 1294-1299: when would this happen?
This is a bit subtle - it was one of the bugs in the original version of the patch. Here's the sequence of events that it covers:
- write 10 bytes to a new file (ie no packet yet)
- call hflush()
- it calls flushBuffer, so that it enqueues a new "packet"
- it sends that packet - now we have no currentPacket, and the 10 bytes are still in the checksum buffer, lastFlushOffset=10
- we call hflush() again without writing any more data
- it calls flushBuffer, so it creates a new "packet" with the same 10 bytes
- we notice that the bytesCurBlock is the same as lastFlushOffset, hence we don't want to actually re-send this packet (there's no new data, so it's a no-op flush)
- hence, we need to get rid of the packet and also decrement the sequence number so we don't "skip" one
Without this fix, we were triggering 'assert seqno == lastAckedSeqno + 1;' when calling hflush() twice without writing any data in between