Unexpected behavior in KIO::MimeTypeFinderJob

[Stefano Crocco]

On mercoledì 7 agosto 2024 19:14:13 CEST Harald Sitter wrote:
> On Wed, Aug 7, 2024 at 6:24 PM David Faure <faure at 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 <faure at 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