ext/ftp: throw an Error when ftp_nb_fget()/ftp_nb_fput() hit a busy connection

[lacatoire]

Reworked as suggested: the guard now throws instead of returning false, so the stub change is gone. The UPGRADING note is kept, rewritten for the new behavior — the warning and false are still going away for callers. Drop it if you would rather not carry the entry.

ftp_nb_fget() and ftp_nb_fput() answered an already busy connection with a warning and false, against a declared int return type. Reaching that guard means a transfer was started from inside another transfer — a programming mistake — so it throws an Error, as ftp_close() already does on the same in_use flag. Both declarations stay int.

The check stays ahead of the direction and closestream writes, so the running transfer is left untouched. The test reaches the guard through a stream wrapper that calls back into the extension mid transfer, so it needs no FTP server beyond the one the ftp tests already start.

ftp_nb_get() and ftp_nb_put() keep their warning and false for now: their int|false is also returned when the local file cannot be opened, so the declaration would not change. Happy to convert their in_use guard too if you want the four consistent.

[ndossche]

Perhaps the code should be revised to throw an Error rather than returning false. Generally, Error is used to indicate a programming mistake, and this is the case here. I believe that's more appropriate than widening the return type

[ndossche]

cc @iliaal

[iliaal]

Agreed, ftp_close() already throws an Error on the same in_use flag. That guard is the only false in ftp_nb_fget() and ftp_nb_fput(), so throwing drops the stub and UPGRADING changes entirely. Keep the check ahead of the direction/closestream writes, and master only.

[ndossche]

This is the right direction, but incomplete.
You should consistently do the same for ftp_nb_get & ftp_nb_put, such that this entire error class (which is rlly a programming error), can be handled in a consistent way.

[ndossche]

Correct me if I'm wrong, but the original fix (that caused this return-type-mismatch issue) went into master-only right? If so, then instead of emitting a warning and returning false against their declared int return type. is confusing and can go away.