Remove busy loop check in TransferJobPrivate::slotDataReqFromDevice
ClosedPublic

Authored by aacid on Aug 1 2017, 6:59 PM.

Details

Summary

We don't need to call ourselves every time to see if the data transfer finished,
the iodevice will signal us thorough readChannelFinished when that happens.

This comes along with an improved test which actually writes data slowly twice,
with the old code this improved test would also fail since after the first write
would decided we are done and finish the transaction.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aacid created this revision.Aug 1 2017, 6:59 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 1 2017, 6:59 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
apol added a subscriber: apol.Aug 1 2017, 10:40 PM

+1
I confirm KDE Connect still works through the QNAM abstraction without going crazy calling slotDataReqFromDevice.

dfaure added a subscriber: dfaure.Aug 9 2017, 7:41 AM

isn't this about the case where there is more data available than MAX_READ_BUF_SIZE, so we didn't read it all and we need to loop around to read the rest?
OK, that's not what the if says, but I think that's still one case where we need to loop. So maybe it should be

if (bytesRead == MAX_READ_BUF_SIZE) {

QIODevice doesn't emit readyRead() again if it notified us about 16K being available and we only read 8K.

aacid added a comment.Aug 9 2017, 9:28 PM

If you read the code (and the test that was failing once i changed the code), no i don't think it is about that.

Do you want me to add a unit test for that scenario?

dfaure added a comment.Aug 9 2017, 9:32 PM

Yes that would be a good idea. AFAICS it's still one good reason to loop around, i.e. it's a possible regression from this patch.

aacid updated this revision to Diff 18097.Aug 13 2017, 5:11 PM

Added new test

aacid added a comment.Aug 13 2017, 5:12 PM

Yes that would be a good idea. AFAICS it's still one good reason to loop around, i.e. it's a possible regression from this patch.

Seems TransferJobPrivate::slotIODeviceClosed takes care of reading the reminder that slotDataReqFromDevice may have not read.

dfaure accepted this revision.Aug 13 2017, 9:32 PM

OK, thanks for the test.

This revision is now accepted and ready to land.Aug 13 2017, 9:32 PM
This revision was automatically updated to reflect the committed changes.