[Twisted-Python] Request.getClient

[Glyph Lefkowitz]

On Jul 2, 2014, at 9:26 AM, exarkun at twistedmatrix.com wrote:

> Hello all,
> 
> twisted.web.http.Request.getClient has a terrible implementation.  It does blocking network I/O (DNS).  Fortunately it is only used in one place in Twisted - the CGI implementation.  Unfortunately this makes the CGI implementation somewhat unsuited for real-world use.
> 
> `Request.getClient` has always been allowed to return `None` under certain circumstances.  I propose making it always return `None` and deprecating it.
> 
> This is implemented in the branch linked to <https://tm.tl/2252>.
> 
> Chris Armstrong suggested that this change might not be strictly keeping with our backwards compatibility policy.

I agree that this is a troubling area but in general I tend to believe that changes like this are in keeping with our compatibility policy.  Changing the signature or the allowed return type of a value ("type" speaking in terms of public features of its interface) should not be allowed.  If we already returned None sometimes, then a correct program would already have to deal with None sometimes, so making this change wouldn't break it, per se.

Bringing up such a change on the list is always a good policy, since it gives people a chance to audit their code and look for places where they might have been depending too intimately on accidental features, so by no means take my belief that this is in-policy to mean that we shouldn't broadly discuss changes like this in the future :-).  Real, actual, broken code is what the policy strives to prevent, so real world code that broke should usually take precedence.

> I suggest that either it is - because `None` was always a possible return value - or that removing the possibility of blocking I/O from applications that are mistakenly using this API makes it worth the not- strictly-compatible change.
> 
> A minor adjustment might be to make it always return the IP address instead, as this was another behavior it previously had.

I think that this adjustment is the best option.  IP addresses are mostly interchangeable with hostnames, so during the transition period while it's being deprecated, even an application relying on this API heavily would at least have an opportunity to keep functionality equivalent during an upgrade.  Making it always return None means that a correct application (one which dealt with the None return value), while not becoming crash-with-an-exception buggy, might lose functionality (logging a source IP of "None" all the time, for example, and losing track of an audit log of who is making what changes).

Unbidden, I have some ideas about how we might preserve even _more_ of the functionality involving DNS lookups, but more effort than just giving back the IP is probably wasted, so I won't mention them.  Let's deprecate the API and move on.

Thanks for bringing this up,

-glyph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20140702/472badae/attachment-0002.html>