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 <v...@mail.mipt.ru
<mailto:v...@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
_______________________________________________
Python-Dev mailing list
Python-Dev@python.org
https://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
