[Python-Dev] Asking for feedback about fixing `ftplib' (issue25458)

[Ivan Pozdeev]

Re: [Python-Dev] Asking for feedback about fixing `ftplib' (issue25458) 
On 28.12.2016 21:02, Giampaolo Rodola' wrote:
>
>
> On Wed, Dec 28, 2016 at 3:57 PM, Ivan Pozdeev <vano at mail.mipt.ru 
> <mailto:vano at mail.mipt.ru>> wrote:
>
>     On 25.12.2016 17:21, Giampaolo Rodola' wrote:
>>     From what I understand the problem should be fixed in urllib so
>>     that it always closes the FTP connection. I understand this is
>>     what happens in recent 3.x versions but not on 2.7.
>     urllib in 2.7 should indeed be fixed to close FTP connections
>     rather than leave them dangling, no question about that.
>
>
> To me it looks like this is the only issue (currently tracked in 
> http://bugs.python.org/issue28931).
So, closing the control connection ungracefully and without checking the 
response is a non-issue? The right way to do it?
>
>     I tried to fix urllib so that it's guaranteed to call voidresp(),
>     and failed. Too complicated, effectively reimplementing a sizable
>     part of FTP client logic is required, perhaps even monkey-patching
>     ftplib to be able to set flags in the right places.
>
>
> Why did you fail? Why urllib can't close() -> voidresp() the FTP 
> session once it's done with it?
A simple voidresp() in a socket close handler doesn't work here. That's 
why I failed.
* Sometimes, the error needs to be ignored, and sometimes, it doesn't. 
With the current architecture, I cannot check which is the case.
* Error checking can't be done in a close handler. Close handlers 
(including this one) are called in finally blocks, raising an exception 
here would disrupt further cleanup actions, leaving objects in an 
undefined state, and mask the current exception if there's any.

What is more important, the entire urllib.ftpwrapper rubs me the wrong way.
urllib's use case is pretty standard. All it does is the basic FTP 
premise of log in, retrieve, log out. So, logically speaking, it 
shouldn't have to implement loads upon loads of custom logic on top of 
ftplib if ftplib is to be called a production-ready library. Any logic 
it has to implement shall be strictly application-specific.

Since it has to, there can only be two possible conclusions: either it 
uses it not in the way intended, or ftplib is an incomplete library and 
is missing some critical parts that are required for practical use.


I won't comment on anything further on that boils down to these two points.
As long as I fail to bring them across, any further discussion is moot.
>
>     With simple commands (|voidcmd|) and self-contained transfer
>     commands (retrlines/retrbinary), ftplib does incapsulate
>     statefulness, by handling them atomically. But with transfercmd(),
>     it returns control to the user halfway through.
>
>
> That's by design. ftplib's transfercmd() just works like that: it's a 
> low level method returning a "data" socket and it's just up to you to 
> cleanly close the session (close() -> voidresp()) once you're done 
> with it. Most of the times you don't even want to use transfercmd() in 
> the first place, as you just use stor* and retr* methods.
>
>     At this point, if ftplib doesn't itself keep track that the
>     control connection is currently in invalid state for the next
>     command, the user is forced to do that themselves instead.
>
>
> Hence why I suggested a docfix.
>
>     And guess what - urllib has to use ftplib through a wrapper class
>     that does exactly that. That's definitely a "stock logic part"
>     'cuz it's an integral part of FTP rather than something specific
>     to user logic.
>
>
> That wrapper should just cleanly close the session.
>
>     The other "stock logic part" currently missing is error handling
>     during transfer. If the error's cause is local (like a local
>     exception that results in closing the data socket prematurely, or
>     the user closing it by hand, or an ABOR they sent midway), the
>     error code from the end-of-transfer response should be ignored,
>     otherwise, it shouldn't.
>
>
> Absolutely not. Base ftplib should NOT take any deliberate decision 
> such as ignoring an error.
> I can even come up with use cases: use ftplib to test FTP servers so 
> that they *do* reply with an error code in certain conditions. Here's 
> a couple examples in pyftpdlib:
>
> https://github.com/giampaolo/pyftpdlib/blob/17bebe3714752b01e43ab60d2771308f4594bd99/pyftpdlib/test/test_functional.py#L1354-L1369
>
> https://github.com/giampaolo/pyftpdlib/blob/17bebe3714752b01e43ab60d2771308f4594bd99/pyftpdlib/test/test_functional.py#L1973-L1980
>
If you claim that testing a server is an intended use case for ftplib 
(rather than meddling in its internals at your own risk), you need to 
document all the other low-level functions that you use (like putcmd) 
for your claim to stand. The currently published interface doesn't offer 
a sufficiently fine-grained control for that use case. Its 
line-of-support is "elementary compounds", none of which leaves the 
control connection in an invalid state.
>
>     Other than making ftplib keep track of the session's internal
>     state while the user has control and providing the custom error
>     handling logic to them, another solution is to never release
>     control to the user midway, i.e. ban transfercmd() entirely and
>     only provide self-contained retr*()/dir()/whatever, but let the
>     callback signal them to abort the transfer. That way, the state
>     would be kept implicitly - in the execution pointer.
>
>
> Banning transfercmd() means renaming it to _transfercmd() which is a 
> no-no for backward compatibility. Also, as I've shown 
> above, transfercmd() does have a use case which relies on it behaving 
> like that (return control midway).
Whyever would a rename be required? More than a few ftplib.FTP's members 
are undocumented outright and they don't start with an underscore.
Anyway, I only mentioned this as one of the possible lines of action.
>
> -- 
> Giampaolo - http://grodola.blogspot.com
>

-- 
Regards,
Ivan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20161229/3389551d/attachment.html>