On mercoledì 7 agosto 2024 19:14:13 CEST Harald Sitter wrote: > On Wed, Aug 7, 2024 at 6:24 PM David Faure <fa...@kde.org> wrote: > > On mercredi 7 août 2024 15:53:37 heure d’été d’Europe centrale Harald > > Sitter> > > wrote: > > > On Wed, Aug 7, 2024 at 12:10 PM David Faure <fa...@kde.org> wrote: > > > > On mercredi 7 août 2024 11:54:20 heure d’été d’Europe centrale David > > > > Faure > > > > > > > > wrote: > > > > > On mercredi 7 août 2024 08:30:43 heure d’été d’Europe centrale > > > > > Stefano > > > > > Crocco > > > > > > > > > > wrote: > > > > > > Hello to everyone, > > > > > > while investigating a bug in Konqueror, I just found what in my > > > > > > opinion is > > > > > > an unexpected behavior of KIO::MimeTypeFinderJob. If I'm reading > > > > > > the > > > > > > code > > > > > > correctly, when it uses KIO::get() to determine the mimetype [1], > > > > > > it > > > > > > lets > > > > > > the TransferJob go on even after it detected the mimetype. > > > > > > > > > > > > The documentation for KIO::get() states: > > > > > > "Special case: if you want to determine the MIME type of the file > > > > > > first, > > > > > > and then read it with the appropriate component, you can still use > > > > > > a > > > > > > KIO::get() directly. When that job emits the mimeType signal, > > > > > > (which > > > > > > is > > > > > > guaranteed to happen before it emits any data), put the job on > > > > > > hold". > > > > > > Since the task of MimeTypeFinderJob is finding the mimetype of the > > > > > > URL, I > > > > > > expected it would put the job on hold as soon as it has determined > > > > > > the > > > > > > mimetype, coherently with the KIO::get() documentation. > > > > > > > > > > Right, that's what KRun::foundMimeType was doing, and I did that in > > > > > MimeTypeFinderJob initially. > > > > > > > > > > But see commit 621585f0e818cd9f75cdd3bcc8665da7ac708283 > > > > > and bug 452729. Seems weird. Talk to Harald ;) > > > > > > > > Of course in KF5 it was a bit different because another process could > > > > resume the slave on hold, made available by klauncher. AFAIK nowadays > > > > that's gone so resuming the slave on hold only works if the *same* > > > > process is going to fetch that URL. I guess that's the issue. If > > > > another > > > > process gets started instead (to open that URL in e.g. an image > > > > viewer), > > > > that slave on hold stays there forever. We need a way to discard it > > > > then, > > > > somehow. > > > > > > I don't see the problem. I mean, the docs are wrong, but beyond that? > > > > > > MimeTypeFinderJob is a KCompositeJob. It subjob()s the TransferJob. > > > subjob calls setParent. When the MimeTypeFinderJob gets deleted the > > > TransferJob gets deleted and that triggers the Scheduler to terminate > > > the Worker process. > > > > You're saying "the current code doesn't leak anything". We all agree on > > that. > > > > The problem with the current code is that it completely removed the > > feature of reusing the same worker for determining the mimetype and for > > actually fetching the data, when both happen in the same process. > > > > Your commit removed that because in the case where both do *NOT* happen in > > the same process, then the slave was kept on hold forever. > > > > So to make everyone happy, we need a way to put a slave on hold, reuse it > > if doing a get() in the same process, and cleaning it up otherwise. > > Ah, I missed that we actually want the holding back. I think > semantically that needs to be expressed as a KCompositeJob of > MimetypeFinderJob and TransferJob? The concern here is that we must > only hold iff there is an actual get() coming so there needs to be a > tie between the two jobs somehow. Otherwise we'd again just hold a > resource lock. e.g. I can have an app that wants to know the mimetype > but not follow it up with a get(). > > HS
Sorry for not answering before. I've had an unexpectedly busy day. Just to be clear: as far as Konqueror is concerned, there's no need of adding back the functionality of using the same worker to fetch the data after determining the mimetype. I just wanted to be sure that MimeTypeFinder didn't go on with retrieving the data after the mimetype had been detected. Looking at the documentation, I expected to see the call to putOnHold() to avoid that and I didn't realize that the call to emitResult would stop the job. Thanks for your answers. Stefano
