[network/kdeconnect-kde] /: Unify code paths for transferring empty and non-empty files
Albert Vaca Cintora <[email protected]>
| Newsgroups | gmane.comp.kde.cvs |
|---|---|
| Message-ID | <[email protected]> |
Git commit d939f1df7cd2aab10e8dd6e3b5fe7adb84643afc by Albert Vaca Cintora.
Committed on 13/08/2026 at 20:02.
Pushed by albertvaka into branch 'master'.
Unify code paths for transferring empty and non-empty files
M +27 -5 core/backends/lan/compositeuploadjob.cpp
M +29 -16 core/backends/lan/landevicelink.cpp
M +18 -1 core/backends/lan/uploadjob.cpp
M +1 -0 core/backends/lan/uploadjob.h
M +11 -1 core/compositefiletransferjob.cpp
M +14 -2 core/filetransferjob.cpp
M +17 -28 plugins/share/shareplugin.cpp
M +141 -0 tests/sendfiletest.cpp
https://invent.kde.org/network/kdeconnect-kde/-/commit/d939f1df7cd2aab10e8dd6e3b5fe7adb84643afc
diff --git a/core/backends/lan/compositeuploadjob.cpp b/core/backends/lan/compositeuploadjob.cpp
index 5530a9f23..32bb42315 100644
--- a/core/backends/lan/compositeuploadjob.cpp
+++ b/core/backends/lan/compositeuploadjob.cpp
@@ -104,15 +104,27 @@ void CompositeUploadJob::startNextSubJob()
// TODO: Create a copy of the networkpacket that can be re-injected if sending via lan fails?
NetworkPacket np = m_currentJob->getNetworkPacket();
- np.setPayload(nullptr, np.payloadSize());
- np.setPayloadTransferInfo({{QStringLiteral("port"), m_port}});
np.set<int>(QStringLiteral("numberOfFiles"), m_totalJobs);
np.set<quint64>(QStringLiteral("totalPayloadSize"), m_totalPayloadSize);
+ const bool hasPayload = np.hasPayload();
+ // This packet is now just a header announcing the transfer to the other end: strip the payload
+ // socket instead of mistaking it for a new file to enqueue.
+ np.setPayload(nullptr, np.payloadSize());
+ if (hasPayload) {
+ np.setPayloadTransferInfo({{QStringLiteral("port"), m_port}});
+ }
+
if (m_device->sendPacket(np)) {
- m_server->resumeAccepting();
- m_timeout.start();
+ if (hasPayload) {
+ m_server->resumeAccepting();
+ m_timeout.start();
+ } else {
+ // Nothing to transfer for this file (e.g. it's empty), the subjob is already done.
+ m_currentJob->start();
+ }
} else {
+ m_running = false;
setError(SendingNetworkPacketFailed);
setErrorText(i18n("Failed to send packet to %1", m_device->name()));
@@ -262,7 +274,13 @@ void CompositeUploadJob::slotResult(KJob *job)
// Copies job error and errorText and emits result if job is in error otherwise removes job from subjob list
KCompositeJob::slotResult(job);
- if (error() || !m_running) {
+ if (error()) {
+ // KCompositeJob::slotResult() already called emitResult() for us in this case.
+ m_running = false;
+ return;
+ }
+
+ if (!m_running) {
return;
}
@@ -272,6 +290,10 @@ void CompositeUploadJob::slotResult(KJob *job)
m_currentJobNum++;
startNextSubJob();
} else {
+ // Mark as finished before emitting the result: a new file added right after this point
+ // (e.g. from another share action) must start a new composite job rather than being
+ // silently attached to this one, which has already stopped picking up new subjobs.
+ m_running = false;
emitResult();
}
}
diff --git a/core/backends/lan/landevicelink.cpp b/core/backends/lan/landevicelink.cpp
index d7f155223..416057776 100644
--- a/core/backends/lan/landevicelink.cpp
+++ b/core/backends/lan/landevicelink.cpp
@@ -59,29 +59,42 @@ QHostAddress LanDeviceLink::hostAddress() const
bool LanDeviceLink::sendPacket(NetworkPacket &np)
{
- if (np.payload() && np.hasPayload()) {
+ // FIXME: Remove packet-type-specific logic from the link
+ if (np.type() == PACKET_TYPE_SHARE_REQUEST && np.payload()) {
+ // A new file to share (even one with no payload, like an empty file, which still has a
+ // payload device attached by SharePlugin::shareUrl() even though its size is 0). This
+ // goes through the composite job so it gets counted and sequenced with the others.
+ // Note: CompositeUploadJob strips the payload device before re-sending this packet as a
+ // header once it's this file's turn, so that re-send falls through to the plain write
+ // below instead of being mistaken for another new file and re-enqueued.
Device *device = Daemon::instance()->getDevice(deviceId());
if (device == nullptr) {
qCWarning(KDECONNECT_CORE) << "Device disconnected" << deviceId();
return false;
}
- // FIXME: Remove packet-type-specific logic from the link
- if (np.type() == PACKET_TYPE_SHARE_REQUEST && np.payloadSize() >= 0) {
- if (!m_compositeUploadJob || !m_compositeUploadJob->isRunning()) {
- m_compositeUploadJob = new CompositeUploadJob(device, true);
- }
-
- m_compositeUploadJob->addSubjob(new UploadJob(np));
-
- if (!m_compositeUploadJob->isRunning()) {
- m_compositeUploadJob->start();
- }
- } else { // Infinite stream
- CompositeUploadJob *fireAndForgetJob = new CompositeUploadJob(device, false);
- fireAndForgetJob->addSubjob(new UploadJob(np));
- fireAndForgetJob->start();
+
+ if (!m_compositeUploadJob || !m_compositeUploadJob->isRunning()) {
+ m_compositeUploadJob = new CompositeUploadJob(device, true);
+ }
+
+ m_compositeUploadJob->addSubjob(new UploadJob(np));
+
+ if (!m_compositeUploadJob->isRunning()) {
+ m_compositeUploadJob->start();
+ }
+
+ return true;
+ } else if (np.payload() && np.hasPayload()) { // Infinite stream
+ Device *device = Daemon::instance()->getDevice(deviceId());
+ if (device == nullptr) {
+ qCWarning(KDECONNECT_CORE) << "Device disconnected" << deviceId();
+ return false;
}
+ CompositeUploadJob *fireAndForgetJob = new CompositeUploadJob(device, false);
+ fireAndForgetJob->addSubjob(new UploadJob(np));
+ fireAndForgetJob->start();
+
return true;
} else {
int written = m_socket->write(np.serialize());
diff --git a/core/backends/lan/uploadjob.cpp b/core/backends/lan/uploadjob.cpp
index 01b826d72..e9eea8ba0 100644
--- a/core/backends/lan/uploadjob.cpp
+++ b/core/backends/lan/uploadjob.cpp
@@ -29,6 +29,16 @@ void UploadJob::setSocket(QSslSocket *socket)
void UploadJob::start()
{
+ if (!m_networkPacket.hasPayload()) {
+ // No actual payload to send (e.g. an empty file): nothing to do, even though m_input may
+ // be a valid-but-empty QIODevice (SharePlugin::shareUrl() always attaches one). Complete
+ // asynchronously (like a real transfer would) instead of synchronously, so we don't
+ // re-enter the composite job's result handling while it may still be in the middle of
+ // having subjobs added to it.
+ QMetaObject::invokeMethod(this, "finishWithoutTransfer", Qt::QueuedConnection);
+ return;
+ }
+
if (!m_input->open(QIODevice::ReadOnly)) {
qCWarning(KDECONNECT_CORE) << "error when opening the input to upload";
return; // TODO: Handle error, clean up...
@@ -80,9 +90,16 @@ void UploadJob::aboutToClose()
emitResult();
}
+void UploadJob::finishWithoutTransfer()
+{
+ emitResult();
+}
+
bool UploadJob::stop()
{
- m_input->close();
+ if (m_input) {
+ m_input->close();
+ }
return true;
}
diff --git a/core/backends/lan/uploadjob.h b/core/backends/lan/uploadjob.h
index 2b2e8ff68..f3557ed4d 100644
--- a/core/backends/lan/uploadjob.h
+++ b/core/backends/lan/uploadjob.h
@@ -41,6 +41,7 @@ private Q_SLOTS:
void uploadNextPacket();
void encryptedBytesWritten(qint64 bytes);
void aboutToClose();
+ void finishWithoutTransfer();
};
#endif // UPLOADJOB_H
diff --git a/core/compositefiletransferjob.cpp b/core/compositefiletransferjob.cpp
index ccfdabc4a..63004d61d 100644
--- a/core/compositefiletransferjob.cpp
+++ b/core/compositefiletransferjob.cpp
@@ -114,7 +114,13 @@ void CompositeFileTransferJob::slotResult(KJob *job)
// Copies job error and errorText and emits result if job is in error otherwise removes job from subjob list
KCompositeJob::slotResult(job);
- if (error() || !m_running) {
+ if (error()) {
+ // KCompositeJob::slotResult() already called emitResult() for us in this case.
+ m_running = false;
+ return;
+ }
+
+ if (!m_running) {
return;
}
@@ -129,6 +135,10 @@ void CompositeFileTransferJob::slotResult(KJob *job)
startNextSubJob();
}
} else {
+ // Mark as finished before emitting the result: a file received right after this point
+ // (e.g. from another share action) must start a new composite job rather than being
+ // silently attached to this one, which has already stopped picking up new subjobs.
+ m_running = false;
emitResult();
}
}
diff --git a/core/filetransferjob.cpp b/core/filetransferjob.cpp
index 39716d234..5e26095de 100644
--- a/core/filetransferjob.cpp
+++ b/core/filetransferjob.cpp
@@ -28,7 +28,8 @@ FileTransferJob::FileTransferJob(const NetworkPacket *np, const QUrl &destinatio
, m_np(np)
, m_autoRename(false)
{
- Q_ASSERT(m_origin);
+ // m_origin can be null when the packet has no payload (e.g. an empty file): we still want to
+ // go through the same job so it gets counted, auto-renamed and timestamped like any other file.
// Disabled this assert: QBluetoothSocket doesn't report "->isReadable() == true" until it's connected
// Q_ASSERT(m_origin->isReadable());
if (m_destination.scheme().isEmpty()) {
@@ -61,7 +62,18 @@ void FileTransferJob::start()
void FileTransferJob::doStart()
{
- if (m_origin && m_origin->bytesAvailable())
+ if (!m_origin) {
+ // No payload to transfer (e.g. an empty file): just create the destination file.
+ QFile file(m_destination.toLocalFile());
+ if (!file.open(QIODevice::WriteOnly)) {
+ setError(1);
+ setErrorText(i18n("Could not create destination file: %1", m_destination.toLocalFile()));
+ }
+ emitResult();
+ return;
+ }
+
+ if (m_origin->bytesAvailable())
startTransfer();
connect(m_origin.data(), &QIODevice::readyRead, this, &FileTransferJob::startTransfer);
diff --git a/plugins/share/shareplugin.cpp b/plugins/share/shareplugin.cpp
index 6534e3523..9b8ae6130 100644
--- a/plugins/share/shareplugin.cpp
+++ b/plugins/share/shareplugin.cpp
@@ -124,36 +124,25 @@ void SharePlugin::receivePacket(const NetworkPacket &np)
const qint64 dateModified = np.get<qint64>(QStringLiteral("lastModified"), QDateTime::currentMSecsSinceEpoch());
const bool open = np.get<bool>(QStringLiteral("open"), false);
- if (np.hasPayload()) {
- if (!m_compositeJob) {
- m_compositeJob = new CompositeFileTransferJob(device(), this);
- m_compositeJob->setProperty("destUrl", destinationDir().toString());
- m_compositeJob->setProperty("immediateProgressReporting", true);
- Daemon::instance()->jobTracker()->registerJob(m_compositeJob);
- }
+ // Every incoming file (even ones with no payload, like empty files) goes through the same
+ // composite job so it gets counted, auto-renamed and timestamped like any other file.
+ if (!m_compositeJob) {
+ m_compositeJob = new CompositeFileTransferJob(device(), this);
+ m_compositeJob->setProperty("destUrl", destinationDir().toString());
+ m_compositeJob->setProperty("immediateProgressReporting", true);
+ Daemon::instance()->jobTracker()->registerJob(m_compositeJob);
+ }
- FileTransferJob *job = np.createPayloadTransferJob(destination);
- job->setOriginName(device()->name() + QStringLiteral(": ") + filename);
- job->setAutoRenameIfDestinationExists(true);
- connect(job, &KJob::result, this, [this, dateCreated, dateModified, open](KJob *job) -> void {
- finished(job, dateCreated, dateModified, open);
- });
- m_compositeJob->addSubjob(job);
+ FileTransferJob *job = np.createPayloadTransferJob(destination);
+ job->setOriginName(device()->name() + QStringLiteral(": ") + filename);
+ job->setAutoRenameIfDestinationExists(true);
+ connect(job, &KJob::result, this, [this, dateCreated, dateModified, open](KJob *job) -> void {
+ finished(job, dateCreated, dateModified, open);
+ });
+ m_compositeJob->addSubjob(job);
- if (!m_compositeJob->isRunning()) {
- m_compositeJob->start();
- }
- } else {
- QFile file(destination.toLocalFile());
- if (file.open(QIODevice::WriteOnly)) {
- file.close();
- setDateCreated(destination, dateCreated);
- setDateModified(destination, dateModified);
- Q_EMIT shareReceived(destination.toString());
- if (open) {
- QDesktopServices::openUrl(destination);
- }
- }
+ if (!m_compositeJob->isRunning()) {
+ m_compositeJob->start();
}
} else if (np.has(QStringLiteral("text"))) {
QString text = np.get<QString>(QStringLiteral("text"));
diff --git a/tests/sendfiletest.cpp b/tests/sendfiletest.cpp
index fef954ba3..f908bc433 100644
--- a/tests/sendfiletest.cpp
+++ b/tests/sendfiletest.cpp
@@ -80,6 +80,51 @@ private Q_SLOTS:
QCOMPARE(file.readAll(), content);
}
+ void testSendEmptyFile()
+ {
+ // An empty file has no payload, but it should still be sent, counted and go through the
+ // same composite job machinery as any other file (fixes empty files being dropped from the
+ // total count, or transfers stalling/crashing when an empty file is mixed in with others).
+ if (!(m_daemon->getLinkProviders().size() > 0)) {
+ QFAIL("No links available, but loopback should have been provided by the test");
+ }
+
+ const auto deviceIds = m_daemon->devices();
+ Device *device = nullptr;
+ for (const QString &deviceId : deviceIds) {
+ Device *d = m_daemon->getDevice(deviceId);
+ if (d->isReachable()) {
+ if (!d->isPaired())
+ d->requestPairing();
+ device = d;
+ }
+ }
+ if (device == nullptr) {
+ QFAIL("Unable to determine device");
+ }
+ QCOMPARE(device->isReachable(), true);
+ QCOMPARE(device->isPaired(), true);
+
+ QTemporaryFile temp;
+ temp.open();
+ temp.close();
+ QCOMPARE(QFileInfo(temp.fileName()).size(), 0);
+
+ KdeConnectPlugin *plugin = device->plugin(QStringLiteral("kdeconnect_share"));
+ QVERIFY(plugin);
+ plugin->metaObject()->invokeMethod(plugin, "shareUrl", Q_ARG(QString, QUrl::fromLocalFile(temp.fileName()).toString()));
+
+ QSignalSpy spy(plugin, SIGNAL(shareReceived(QString)));
+ QVERIFY(spy.wait(2000));
+
+ QVariantList args = spy.takeFirst();
+ QUrl sentFile(args.first().toUrl());
+
+ QFile file(sentFile.toLocalFile());
+ QVERIFY(file.exists());
+ QCOMPARE(file.size(), 0);
+ }
+
void testSslJobs()
{
const QString aFile = QFINDTESTDATA("sendfiletest.cpp");
@@ -130,6 +175,102 @@ private Q_SLOTS:
QCOMPARE(resultFile.readAll(), originFile.readAll());
}
+ void testUploadEmptyFile()
+ {
+ // A packet with no payload set at all (like the ones sent for empty files) must still
+ // flow through CompositeUploadJob/UploadJob as a normal, immediately-completing subjob,
+ // so it gets counted instead of being silently dropped from the total.
+ const QString destFile = QDir::tempPath() + QStringLiteral("/kdeconnect-test-empty-upload");
+ QFile(destFile).remove();
+
+ DeviceInfo deviceInfo = KdeConnectConfig::instance().deviceInfo();
+ KdeConnectConfig::instance().addTrustedDevice(deviceInfo);
+
+ Device *device = new Device(this, deviceInfo.id);
+ m_daemon->addDevice(device);
+
+ NetworkPacket np(PACKET_TYPE_SHARE_REQUEST);
+ np.set<QString>(QStringLiteral("filename"), QStringLiteral("empty.txt"));
+ QVERIFY(!np.hasPayload());
+
+ CompositeUploadJob *job = new CompositeUploadJob(device, false);
+ UploadJob *uj = new UploadJob(np);
+ job->addSubjob(uj);
+
+ // Like testSslJobs(), sending the packet over the (fake) device link is not actually
+ // exercised here; we only check that the no-payload subjob completes on its own instead
+ // of hanging forever waiting for a socket connection that will never come.
+ QSignalSpy spyUpload(job, &KJob::result);
+ job->start();
+
+ QVERIFY(spyUpload.count() || spyUpload.wait());
+ // Once finished (however it finished), the job must not be mistaken for still-usable by
+ // whoever holds onto it (e.g. LanDeviceLink), or a later file would be silently attached
+ // to this already-finished job and never actually get sent.
+ QVERIFY(!job->isRunning());
+
+ FileTransferJob *ft = np.createPayloadTransferJob(QUrl::fromLocalFile(destFile));
+ QSignalSpy spyTransfer(ft, &KJob::result);
+ ft->start();
+
+ QVERIFY(spyTransfer.count() || spyTransfer.wait());
+ QCOMPARE(ft->error(), 0);
+
+ QFile resultFile(destFile);
+ QVERIFY(resultFile.exists());
+ QCOMPARE(resultFile.size(), 0);
+ }
+
+ void testMultipleEmptyFilesInOneCompositeUpload()
+ {
+ // Regression test for a bug where sending several empty files in one go got the count
+ // wrong and the transfer stuck: for each file, CompositeUploadJob sends a per-file
+ // "header" packet to announce the transfer, and that packet must go straight out over
+ // the wire rather than be mistaken for yet another new file to enqueue. For files with a
+ // payload, the header's payload device was explicitly stripped before sending; for files
+ // with no payload (like empty files, which still carry a valid-but-empty QIODevice per
+ // SharePlugin::shareUrl()) it wasn't - so LanDeviceLink treated it as a new file, feeding
+ // it back into the same composite job and leaving it stuck waiting for a subjob that
+ // would never actually arrive (see bug report: 3 empty files shown as "1 of 4", stuck).
+ DeviceInfo deviceInfo = KdeConnectConfig::instance().deviceInfo();
+ KdeConnectConfig::instance().addTrustedDevice(deviceInfo);
+
+ TestDevice *device = new TestDevice(this, deviceInfo.id);
+
+ QTemporaryFile empty;
+ empty.open();
+ empty.close();
+
+ CompositeUploadJob *job = new CompositeUploadJob(device, false);
+ for (int i = 0; i < 3; ++i) {
+ QSharedPointer<QFile> f(new QFile(empty.fileName()));
+ NetworkPacket np(PACKET_TYPE_SHARE_REQUEST);
+ np.setPayload(f, 0);
+ np.set<QString>(QStringLiteral("filename"), QStringLiteral("empty%1.txt").arg(i));
+ QVERIFY(!np.hasPayload());
+ job->addSubjob(new UploadJob(np));
+ }
+
+ QSignalSpy spyUpload(job, &KJob::result);
+ job->start();
+
+ QVERIFY(spyUpload.count() || spyUpload.wait());
+ QCOMPARE(job->error(), 0);
+ QVERIFY(!job->isRunning());
+
+ // Exactly one header packet per file must have gone out - not fewer (stuck) and not more
+ // (a file's own header being mistaken for another new file).
+ QCOMPARE(device->getSentPackets(), 3);
+
+ NetworkPacket *last = device->getLastPacket();
+ QVERIFY(last);
+ QCOMPARE(last->type(), QString(PACKET_TYPE_SHARE_REQUEST));
+ QCOMPARE(last->get<int>(QStringLiteral("numberOfFiles")), 3);
+ // The header must not carry the payload device onward: it's just an announcement, not
+ // something the other end should try to open a transfer socket for.
+ QVERIFY(!last->payload());
+ }
+
void testMoreDataThanAnnounced()
{
// A sender can announce a size that turns out to be smaller than what it then sends,