On 03/14/2016 06:17 AM, Amos Jeffries wrote:
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 Yn
action already having finished. If both X and Y have finished, then
schedules the finalization. Otherwise just exits the call chain.
Yep.
But:
- the master->userDataDone is used only in fssHandleDataRequest case,
and in all other cases will set to false causing bugs if moved out of an
"if (fssHandleDataRequest)".
- The completeDataExchange it is required in the fssHandleDataRequest
case. Of course the suggested block "if (delayedReply) {} else {}" will
work and looks better. But just my opinion is that at least for now keep
completeDataExchange into an "if(fssHandleDataRequest)" block. Else we
need to rename completeDataExchange and change it a little.
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.
yep.
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.
fssHandleDataRequest
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/
The correct looks that it is the "if originWaitInProgress"
* double empty line after originWaitInProgress. please reduce to 1 line.
ok
* 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?
You are right here. This method is called after one downloading (not
uploading) is finished and we received from FTP server a control code
about downloading status.
The data channel is always used for one object download/upload.
Is it ok to just add a debugs like the following:
+ debugs(9, 2, "Complete data downloading");
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.
I am waiting your and Alex comments before proceed.
Amos
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
