-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relax Plack::Util::content_length checks #439
Comments
|
I think from what I understand based on our previous discussion and your catalyst advent article is that in cases where people take advantage of the idea that the PSGI $body can be 'file handle like' and return something that isn't a real file handle but nevertheless provides some ability to determine size, it would be ideal if the contentLength middleware could use that. To give all some context, the HEAD of Catalyst branch runner now uses Plack::Middleware::ContentLength instead of its custom behavior. In general PM::ContentLength is better but I think we could improve it by letting it guess harder a bit. |
|
The current code fallback for elsif uses Plack::Util::is_real_fh which contains checks for a valid file descriptor. This is notably used in Twiggy::Server to define the difference between a handle that can be used for AIO and one that will otherwise implement 'getline'. The case here is that function qualifying whether the -s filestat will be used is overkill and the -s test should be enough. As noted we can have a handle with an overloaded response to -s even though the in memory nature means there is no file descriptor. Coderefs would be expected to stream. |
|
There're a couple of problems with the original content_length code which you seem to have overlooked. Firstly, if $body is a real filehandle, but to a pipe, fifo, or socket, and if it has data pending, then -s will, on some platforms, return the number of bytes which haven't yet been read. For any of those types of handles, content_length really ought to return undef. Second, if $body is a handle opened via open($fh, "<", "string to be read"), then -s on it will return undef (at least as of 5.16). However, it's size can be gotten, since this kind of handle responds to tell and seek just like a normal handle. While I'm not entirely sure why someone would use such a handle with PSGI (other than testing), we probably ought to handle it properly. Conveniently, perldoc -f tell says that it usually fails (returns -1) on pipes, fifos, and sockets, which means we can use it to identify which handles we can safely pre-calculate a length. Thus: .... # other code the same. Thus, we only use -s if tell() had worked, or if -X is overloaded. |
Currently Plack::Util::content_length calls Plack::Uril::is_real_fh before attempting a filestat for the size. 'is_real_fh' has it's purposes in many cases but I believe it is valid for something to respond to a filestat -s while not complying to everything 'is_real_fh' will test for.
The following should work without warnings :
sub content_length {
my $body = shift;
}
The text was updated successfully, but these errors were encountered: