D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Fu Sitter]

fusitter added a comment.


  You think sending your minions to insult me and then disabling my account 
will solve the issue? what will you do next? Disable registration so no one 
points out your hypocrisy? Which overlord made the decision to disable my 
account and for what?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: fusitter, ngraham, rikmills, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Yaroslav Sidlovsky]

ZaWertun abandoned this revision.
ZaWertun added a comment.


  No problem. I'm closing revision then.
  Thanks for the feedback.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: ngraham, rikmills, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Harald Sitter]

sitter added a comment.


  Well, we *could* land it, I am also not sure if that is a net-improvement 
though. As David said we are playing ping-pong with bugs and all of them are 
symptoms of the larger issue that error handling is done inconsistently, which 
in turn is why fixing one bug causes another, as has happened here. This will 
keep on happening until we address the root problem. If you want to help with 
facilitating that what I really want is a quick and dirty integration test 
system built for ftp so we can check such high level functionality as "can I 
overwrite a file".

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: ngraham, rikmills, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Nathaniel Graham]

ngraham added a comment.


  > but sure, whatever, if you think it's better with this commit than with the 
earlier equally broken two versions, I'm not objecting.
  
  @sitter so what do you think, should we land this in the meantime?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: ngraham, rikmills, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Yaroslav Sidlovsky]

ZaWertun added a comment.


  Just found another bug on the KDE bugzilla related to this: 
https://bugs.kde.org/show_bug.cgi?id=410357.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: rikmills, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Harald Sitter]

sitter added a comment.


  In D22528#507373 , @dfaure wrote:
  
  > Is your work on a proper two-classes solution not ready?
  
  
  Nope, I haven't made any progress on it beyond what I posted. I briefly 
looked at a testing setup to make sure stuff doesn't regress but that got me 
discouraged again :|

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: rikmills, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[David Faure]

dfaure added a comment.


  I can't say if this is better in or out, since it's broken in any case. We're 
playing ping-pong with broken code, but sure, whatever, if you think it's 
better with this commit than with the earlier equally broken two versions, I'm 
not objecting.
  
  Is your work on a proper two-classes solution not ready?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: rikmills, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Harald Sitter]

sitter added a comment.


  @dfaure are we ok to land this for now?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: rikmills, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Yaroslav Sidlovsky]

ZaWertun added a comment.


  Wrong. Still present in KF frameworks 5.61:
  
  KIO’s FTP connection feature is now more tolerant of broken FTP server 
implementations (Enes Selim, KDE Frameworks 5.61)
  
  (from 
https://pointieststick.com/2019/07/28/kde-usability-productivity-week-81/)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: rikmills, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Yaroslav Sidlovsky]

ZaWertun added a comment.


  Bug still present in plasma 5.16.4.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: rikmills, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[David Faure]

dfaure added a comment.


  I like very much your proposal about using two classes to avoid reintroducing 
problems in the future.
  
  I like a lot less the use of exceptions, generally speaking... one uncaught 
exception and it's the end of the world.
  But OK I see that in this case, there is a clear layer where all exceptions 
are caught, so this typical pitfall is avoided. I guess I can make an exception 
to my rule about exceptions, haha :-)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Harald Sitter]

sitter added a comment.


  Something like this https://phabricator.kde.org/P439

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Harald Sitter]

sitter added a comment.


  Hm. I've had a quick look and I think the intent was that only the slavebase 
overridden functions call error or finished. So technically all internal 
functions that can have an error need to set an iError or return one so that 
the "public" function can then issue error() as needed. This seems to not very 
well enforced and probably never was, in fact there's a comment in the .h about 
this very fact
  
// 
// All the methods named ftpXyz are lowlevel methods that are not exported.
// The implement functionality used by the public high-level methods. Some
// low-level methods still use error() to emit errors. This behaviour is not
// recommended - please return a boolean status or an error code instead!
// 
  
  What I think may be best to resolve this is to split the class in two.
  
  The "public" class only has overrides of SlaveBase. It's functions are the 
only ones that may issue `error()` or `finished()`.
  The "private" class has all the supporting functions and has no access to 
SlaveBase so it cannot issue error or finished.
  This should be a very durable solution as even in the future no one will 
accidentally add another call to error or finished in one of the internal 
functions since they are in a different object and all error handling must be 
done via iError or some other system.
  
  In fact, I am thinking this is a case where one could just use exceptions. 
The private class could throw a simple exception on errors and in the public 
class we'd need nothing more than a try{}catch in all the public functions.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[David Faure]

dfaure added a comment.


  Please wait.
  
  This is a partial revert of my earlier commit to this file -- not exactly, 
but it feels like there is a bit of a mess regarding the question of whether 
error() was already emitted or not.
  I made the assumption that statusServerError meant error was already emitted 
(by earlier function calls) but it now seems this assumption was wrong.
  But then it feels like we're playing ping-pong with this code in a way that 
will never be satisfactory, if various error cases have or have not already 
emitted error() at this point.
  This needs further research and cleanups.
  
  Well, presumably we're better off with this patch in than out, so actually if 
you want to commit, I (or someone else...) can do the research either way.
  
  Does anyone know which methods are supposed to call error() and which methods 
are supposed to just set iError and let the caller do it? Right now it seems to 
be a bit of a mess...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Yaroslav Sidlovsky]

ZaWertun added a comment.


  In D22528#497803 , @sitter wrote:
  
  > Looks good to me. The slave indeed must issue an exit state there.
  >
  > What email address would you like to have associated with the git commit?
  
  
  zawer...@gmail.com
  
  Thanks!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Harald Sitter]

sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  Looks good to me. The slave indeed must issue an exit state there.
  
  What email address would you like to have associated with the git commit?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Yaroslav Sidlovsky]

ZaWertun added a comment.


  Patch was made against version 5.60.0 (rev 8513ca9 
).
  Sorry that I didn't mention it earlier.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Nathaniel Graham]

ngraham added a reviewer: cfeck.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22528: KIO FTP: Fix file copy hanging when copying to existing file

[Nathaniel Graham]

ngraham retitled this revision from "KIO FTP: File copy hangs when copying to 
existing file - fix" to "KIO FTP: Fix file copy hanging when copying to 
existing file".
ngraham edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22528

To: ZaWertun, sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns