Re: [PATCH] Bug 3643: Connection Auth redesign

On 03/27/2013 05:38 AM, Amos Jeffries wrote:
> On 27/03/2013 10:16 a.m., Alex Rousskov wrote:
>> On 03/25/2013 09:59 PM, Amos Jeffries wrote:
>>> +private:
>>> + /// state of some credentials that can be used to perform
>>> authentication on this connection
>>> + Auth::UserRequest::Pointer credentialsState;

>> The words "state", "data", and "info" usually do not add to the
>> description or understanding of a member. How about renaming
>> "credentialsState" to just "credentials"?
>
> "UserRequest" is not just the credentials. It is the credentials along
> with all the additional state information. ie how, when and if they
> validated, any protocol options line realm which are relevant, which
> helper was used if stateful, the HelperReply line details etc. whatever
> the potocol scheme is written to preserve.

Yes, of course. It is possible that "credentials" itself is wrong. As is
UserRequest, apparently :-).

> I think this is possibly one case where saying state is actually
> warranted.

Not in my opinion. The word "state" does not add any meaning in this
context. Pure credentials without all that extra stuff would still be
"state" (just a smaller one). This is, of course, not as bad as
something like naming a class implementing a protocol client
"ServerStateData" :-).

> Used getAuth() / setAuth() / auth_ since its more than just credentials.

Looks good to me, thanks!

>>> + // clobbered with alternative credentials
>>> + if (aur != credentialsState) {
>>> + debugs(33, 2, HERE << "ERROR: Closing " << clientConnection << " due to change of connection-auth from " << by);
>>> + credentialsState->releaseAuthServer();
>>> + credentialsState = NULL;
>>> + unpinConnection();
>>> + // this is a fatal type of problem. Close the connection immediately
>>> + clientConnection->close(); // XXX: need to send TCP RST packet if we can.
>>> + stopReceiving("connection-auth change attempted");
>>> + return;
>>> + }

>> IIRC, we should either stopReceiving() or close the connection. Doing
>> both is conceptually wrong because stopReceiving() implies that we
>> should keep sending if needed (which connection closure prohibits) and
>> because connection closure already implies that we will not receive
>> anything. Doing both may also lead to confusing double-closures.

> For this case we need to close the connection ASAP and data loss of
> everything buffered for this connection is the desired outcome.

Let's call comm_reset_close() then (directly or indirectly, see below).

> The stopReceiving() is to both speed up the signalling that the client
> connection has closed, and produce a nice error message about why.
> Are you aware of any better way to exit ConnStateData with an error?

There is no API to do that and we should not abuse stop*() API. Our
choices are:

  1) Just use comm_reset_close().
     Fastest closure possible but no nice error message.

  2) Add an optional reason to comm_reset_close().
     Fastest closure possible, with an error message.
     Perhaps make this a Connection method?

  3) Add ConnStateData::close(message, reset) or similar method.
     Fastest closure possible, with a context-aware error message.

Please note that we should not use ConnStateData::mustStop() because
setAuth() may not be under job call protection. We have to close the
connection to kill our job, unfortunately.

> NP: Comm::Connection::close() can be called any number of times and
> protects against repeated comm_close() cycles if that was what you mean?

Yes, I know, but that protection does not make repeated closures less
confusing.

> I am also having second thoughts about whether the unpin and
> stopReceiving side effects matter when called from swanSong(). That
> comment and code was written for the mk1 patch which used deleteThis in
> the abort case (and crashed sometimes as a result). What do you think
> about just calling setAuth(NULL) there?

Well, it is difficult to suggest the best action because the need for
releaseAuthServer() API itself is a sign of a problem -- the release
should be done automatically (by default) when UserRequest is destroyed.
However, I am sure it is difficult to fix that.

If a proper solution is too difficult, how about adding a private
ConnStateData::endAuth() method that will do this:

+ auth_->releaseAuthServer();
+ auth_ = NULL;
+ unpinConnection();

and then call if from setAuth() and from swanSong(), as needed? This way
any changes to that cleanup code will be in one place.

BTW,

1) Why are we unpinning the connection in setAuth()? Seems like that is
not directly related to authentication. Can that call wait until we
close the connection and the regular cleanup takes place?

2) How come ConnStateData::swanSong() does not call unpinConnection()?
Are we sometimes leaking pinning.peer and pinning.host because of that?

Cheers,

Alex.
Received on Wed Mar 27 2013 - 18:50:56 MDT