bpo-2628: support BLOCK mode for retrbinary #29337
Conversation
| # shutdown ssl layer | ||
| if _SSLSocket is not None and isinstance(conn, _SSLSocket): | ||
| conn.unwrap() | ||
| self.voidcmd('MODE %s' % self.transmissionmode) |
There was a problem hiding this comment.
Since I expect few servers support BLOCK mode, I would explicitly send "MODE" command only if self.transmissionmode != S
| @@ -84,7 +117,11 @@ def push(self, what): | |||
| self.baseclass.next_data = None | |||
| if not what: | |||
| return self.close_when_done() | |||
| super(DummyDTPHandler, self).push(what.encode(self.encoding)) | |||
|
|
|||
| if type(what) != bytes: | |||
There was a problem hiding this comment.
suggested change: isinstance(what, bytes)
|
|
||
| Currently, the mode set is applied only when a client calls retrbinary. | ||
| """ | ||
| self.transmissionmode = mode |
There was a problem hiding this comment.
It probably makes sense to raise ValueError if specified mode is not in ("S", "B").
There was a problem hiding this comment.
Also, does the RFC say anything about what should happen if you switch back to STREAM mode from BLOCK mode (knowing RFC-959 myself, I imagine not)? Should the dataconn socket be closed?
| """Close the persistent data connection.""" | ||
| if self.dataconn: | ||
| self.dataconn.close() | ||
| self.dataconn = None |
There was a problem hiding this comment.
You should call this also from close() method (important).
Also, "data connection" is a very well established concept in FTP, but 99% of the times applies to STREAMing mode. I would be more explicit and call this method _close_block_dataconn or _close_persistent_dataconn.
| elif self.transmissionmode == 'B': | ||
| with self.transfercmd(cmd, rest) as conn: | ||
| while 1: | ||
| # Receive one byte at a time, not all 3 at once -- seriously |
There was a problem hiding this comment.
Why? Is it because returned data may be < 3 bytes?
| print('*suspect data*') | ||
| else: | ||
| self.close_dataconn() | ||
| raise NotImplementedError |
There was a problem hiding this comment.
I suggest ValueError("Invalid transmission mode %r" % self.transmissionmode)
| @@ -495,6 +588,8 @@ def storbinary(self, cmd, fp, blocksize=8192, callback=None, rest=None): | |||
| The response code. | |||
| """ | |||
| self.voidcmd('TYPE I') | |||
| self.voidcmd('MODE S') | |||
There was a problem hiding this comment.
Since I expect few servers to implement BLOCK mode, I would avoid sending MODE command unless it's really necessary. I would change this to:
if self.transmissionmode != 'S':
# BLOCK mode only applies to RETR. STOR does not support it.
self.voidcmd('MODE S')
|
|
||
| Specifies the mode in which to transmit files retrieved in BINARY transfer mode. | ||
| Legal values per RFC 959 are 'S' (STREAM, the default), 'B' (BLOCK) and | ||
| 'C' (COMPRESSED). This library supports STREAM and BLOCK modes only. |
There was a problem hiding this comment.
You should probably mention that BLOCK is supported for RETR (donwload) only.
| if self.debugging: | ||
| print("*got more data than desired!* %d vs %d" % (len(buff), blocklength-sread)) | ||
| sread = sread + len(buff) | ||
| data = data + buff |
There was a problem hiding this comment.
Adding (+) chunks of bytes on every iteration is not very efficient. You could use a bytearray instead. Not tested:
buff = bytearray()
curr_length = blocklength
while len(buff) < blocklength:
chunk = conn.recv(curr_length)
buff.extend(chunk)
curr_length -= len(chunk)
data = bytes(buff)In order to avoid making this method too complex, perhaps it makes sense to encapsulate this logic in a utility function (again, not tested):
def _read_until(sock, length):
if length <= 0:
return b""
buff = bytearray()
curr_length = length
while len(buff) < length:
chunk = sock.recv(curr_length)
buff.extend(chunk)
curr_length -= len(chunk)
return bytes(buff)There was a problem hiding this comment.
Also, if the server sends us a big number (say size = 1G), we will hog RAM. I wonder if it would make sense to implement an additional, internal buffer, that calls callback function after a certain threshold (e.g. 1M) and empties the internal memory buffer.
| @@ -105,6 +106,8 @@ class FTP: | |||
| passiveserver = True | |||
| # Disables https://bugs.python.org/issue43285 security if set to True. | |||
| trust_server_pasv_ipv4_address = False | |||
| transmissionmode = 'S' | |||
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
bpo-2628 was closed, should this be as well? |
https://bugs.python.org/issue2628
https://bugs.python.org/issue2628