Stop reading a dataset once the client is gone - #107
Merged
Conversation
The three MVS send loops in ftpd_mvs_retr() discarded the return value of ftpd_data_send(). When the data connection died mid-transfer the loop kept going: it read every remaining record from DASD and issued a doomed send() for each one, until end of dataset. The JES and UFS paths have always tested the return; MVS datasets were the outlier. Measured on a RETR of a 2693-track data set whose client stopped reading and vanished: the session stayed active for about a minute and a half afterwards, grinding through the rest of the data set for a client that was not there. Nothing leaked -- it finishes -- but it finishes by wasting the I/O, holding a worker, and writing one error line per record, since ftpd_data_send() logs every failure. All three loops now break on a negative return, so the transfer stops where the connection did. The completion path told the same untruth. It replied 250 "Transfer completed successfully" unconditionally, so an aborted transfer looked like a good one to the client and in the log, and the byte counters included records that never left the machine. An aborted transfer now answers 426 with the number of bytes that actually went out, and the log line says so too. Because the loops break, total is once again the honest count. Fixes #106
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #106
The change
The three MVS send loops in
ftpd_mvs_retr()(TYPE I, A and E) now break whenftpd_data_send()returns negative, so a transfer stops where the connection did instead of reading the data set to EOF for a client that is no longer there. The JES and UFS paths have always done this; MVS datasets were the outlier.The completion path told the same untruth and is fixed with it: it replied
250 Transfer completed successfullyunconditionally, so an aborted transfer looked like a good one both to the client and in the log, and the byte counters included records that never left the machine. An aborted transfer now answers426with the number of bytes that actually went out, and the log line saysaborting/ABORTEDrather thandone. Because the loops break,totalis once again the honest count feedingsess->bytes_sentandtotal_bytes_out.Verified live on MVS
Build
50F1D64(this branch), activated intoFTPD.V1R0M0.LINKLIB, STC restarted — the startup line confirms the running module:FTPD000I FTPD 1.0.1-DEV (50F1D64).The case from #106 —
RETR SMPBKUP.SMPPTS(2693 tracks, ~51 MB), client stops reading and then drops the data connection:250 Transfer completed successfully.426 Transfer aborted: data connection lost after 228672 bytes.F FTPD,SESSIONSright afterwards:0 / 10. The control connection survives the aborted transfer —QUITstill answered221— which is correct: a dead data connection is not a dead session.Regression, normal transfers still report 250:
FTPD.SAMP.XMITread to completion →250, 10000 bytes.250, 7972 bytes (shorter than binary because ASCII mode trims trailing blanks and adds CRLF — expected).250, 0 bytes. Checked that this is genuinely an empty data set (GET /zosmf/restfiles/ds/IBMUSER.CURL.MALFreturns 0 bytes) and not the new break firing early.Scope note
ftpd_ufs_retr()(src/ftpd#ufs.c:465) breaks correctly on a send error but then still replies226 Transfer complete— the same "aborted transfer reported as successful" flaw this PR fixes for MVS datasets. Left alone deliberately to keep this change to one file and one path; happy to follow up if wanted.