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