Unexpected behavior in KIO::MimeTypeFinderJob

[Harald Sitter]

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

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.
>
> --
> David Faure, faure at kde.org, http://www.davidfaure.fr