On 14/03/2016 8:57 a.m., Christos Tsantilas wrote: > Hi all, > > I made all of the fixes requested by Alex. > Please see below for my comments. > > On 03/10/2016 11:35 PM, Alex Rousskov wrote: >> On 03/10/2016 12:14 PM, Christos Tsantilas wrote: >> >>> if (master->serverState == fssHandleDataRequest) { >>> + if (!master->userDataDone) { >> ... >>> + originDataDownloadAbortedOnError = (originStatus > 400); >>> + return; >>> + } >>> + >>> + completeDataExchange(); >>> + } else { >>> + if (delayedReply != NULL) { >>> + writeForwardedReply(delayedReply.getRaw()); >>> + delayedReply = NULL; >>> + } >>> + } >> >> >> The above logic looks correct to me, but I feel like I am reading an >> inside-out code. Please consider this instead: >> >> // HttpReply in fssHandleDataRequest lacks status code >> if (master->serverState == fssHandleDataRequest) >> originDataDownloadAbortedOnError = (originStatus > 400); >> >> if (!master->userDataDone) { >> ... your big comment here ... >> // HttpReply in fssHandleDataRequest lacks status code >> if (master->serverState == fssHandleDataRequest) >> originDataDownloadAbortedOnError = (originStatus > 400); >> >> debugs(33, 5 "too early to write the response"); >> return; >> } >> >> if (delayedReply) { >> writeForwardedReply(delayedReply.getRaw()); >> delayedReply = nullptr; >> } else { >> completeDataExchange(); >> } > > I did not follow your suggestion here.
It might be easier to see in a simpler context. I believe Alex is suggestign you use a design closer to the stopReading()/stopWriting() methods in ConnStateData. Each of those methods sets its end-of-X actions, then checks for Y action already having finished. If both X and Y have finished, then schedules the finalization. Otherwise just exits the call chain. > The completeDataExchange should be called only if we are downloading > data. The exception case is the fssHandleDataRequest case so I am > feeling that it is better to have a block with this case. > But I adjusted the block. > > Also: > - The master->userDataDone is adjusted now inside the > Ftp::Server::userDataCompletionCheckpoint, not inside > completeDataExchange method > - I split my big commend to one comment inside > Ftp::Server::stopWaitingForOrigin and an other small comment inside > Ftp::Server::userDataCompletionCheckpoint where the master->userDataDone > is updated. > > > Probably the completeDataExchange should be renamed to > completeDataDownload. It is used only while we are downloading objects > (not uploading). > Agreed "exchange" sounds like a whole-transaction thing. Not a one-way completion. >> >> All of these are pretty much cosmetic changes that do not alter the >> proposed patch functionality AFAICT. IMO, if there are no objections, >> the polished patch should go into trunk (see above regarding v3.5). > > I am attaching a new patch here, if no objection I will apply this to > trunk. > The below is an audit of the code comments and style only: * stopOriginWait() does not make sense in English. - s/if waits/if originWaitInProgress/ - OR, s/if waits/if waiting for origin/ * double empty line after originWaitInProgress. please reduce to 1 line. * readTransferDoneReply() its not clear what the moved debugs() is saying - is it finishing use of a data channel? - is it finished one object send/receive on a data channel that had (or will have) multiple transfers happening on it? - is the new location of the debugs still agnostic to up/down direction of transfer? I'm not going to +1 this right now because I'm not clear on what the logic should be. Consider this a 'no objection' from me. Amos _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev