Skip to content
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

Support proper handling of `STDERR` #27

Closed
andybee opened this Issue Jan 29, 2019 · 12 comments

Comments

Projects
None yet
3 participants
@andybee
Copy link

commented Jan 29, 2019

Expected Behaviour

STDERR response values be uniquely extractable from a response, like headers and body are.

Actual Behaviour

STDERR is identified in fetchResponse() of Socket.php, but never handled independently and ultimately gets merged in to the raw response. This means for a typical FPM response we end up with something like:

Status: 200
PHP message: Error occurred in library
Additional line about error
Another additional line
X-HTTP-Header: Foo
X-Another-HTTP-Header: Bar

Body

Where the first part is actually STDERR, the second part is the headers and the third part the body.

This then, in turn, breaks the header parser as it will parse the first 2 lines as headers and return the remaining lines of the STDERR message along with all HTTP headers as part of the body.

Steps to Reproduce the Problem

  1. Setup fast-cgi-client against an instance of PHP-FPM.
  2. Create a PHP script that includes a error_log('blah'); call.
  3. Observe $this->rawResponse inside the Response object.

Specifications

  • Package version: 2.5.0
  • PHP version: 7.2

Further comments

I have attempted to put together a fix for this, but I'm unsure of the best approach inside Socket and, in turn, Response to store and then surface this data independent of the actually "response".

@andybee andybee changed the title Support proper handling of stderr Support proper handling of `STDERR` Jan 29, 2019

@andybee

This comment has been minimized.

Copy link
Author

commented Jan 29, 2019

Realised this is more related to #26, closing this issue.

@andybee andybee closed this Jan 29, 2019

@hollodotme

This comment has been minimized.

Copy link
Owner

commented Jan 30, 2019

Re-opening for discussing the approach from #26 (comment)

  • separate STDOUT and STDERR output in pass through callbacks
  • separate STDOUT and STDERR in the Response

In detail this could mean for a v2.6.0:

  1. The pass through callback function declaration gets a second argument:
    $callback = function( string $outputBuffer, string $errorBuffer ) {};
  2. The Response gets two new methods getOutput() (which is identical to getRawResponse() and returns the output from STDOUT) and getError() which provides the output from STDERR
  3. getRawResponse() could be deprecated in favor of consistent naming, and removed in v3.0.0.
  4. The newly added ProcessManagerException will be restricted to the Primary script unknown error, as this definitely is unexpected behaviour in any case, IMO.

Once this done users can decide whether to throw exceptions on error output in the response or not.

@andybee

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

I like this approach, and agree that Primary script unknown is a "proper" exception-like state.

It's actually very similar to what I hacked together for our use case, but better thought through 👍

@theseer

This comment has been minimized.

Copy link

commented Jan 30, 2019

I also generally like the ideas outlined in 1 to 3. They make a lot of sense to me and, after reading it, it's almost surprising that it wasn't implemented like that to begin with ;). But one of course always knows better later on...

I'm not fully sure about 4 though, but that might be due to lack of detailed knowledge. While my - and probably that of the majority of the users - use case is to use PHP to talk to PHP-FPM, {the|a} fast-cgi-client of course should not restrict itself to talking to PHP-FPM. Afterall, it's not a PHP-FPM-Client ;)

It feels a bit like an impossible situation: We want the client library to detect errors. But what constitutes as an error that the client should translate into an exception and what does, from the client's perspective, qualifies as a valid result, even if that might very well be an error or error message.

I guess what I'm trying to say is: Would, in http context, a 500 Error response qualify as the webserver being available or not? After all, it replied. The fact the server couldn't handle the request properly is a totally different issue. Granted, the answer is not very usable but again, that's also a different issue.

The case for PHP-FPM replying with "Primary script unknown" looks like the same type of problem to me.

Yet, from a users perspective, I'd argue that indeed the request failed. So, having an exception would make sense. But is that the only case? What about "premature end of script headers" as nginx likes to complain about in case the fcgi server process crashes? What about non-php-fpm fcgi end points?

@andybee

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

One thing that's probably worth noting in here, when I get the 'Primary script unknown' error message I also get a 404 style response. It seems common to always stash, but not necessarily display, STDERR when you also have STDOUT. But maybe I'm misinterpreting what's going on?

@theseer

This comment has been minimized.

Copy link

commented Jan 30, 2019

@andybee The Problem that triggered the initial change was that trying to call a path with relative fragments like .. was more or less silently not working. That's again different than using a regular path that simply doesn't exist.

Or so I believe at least.

@hollodotme hollodotme self-assigned this Feb 6, 2019

@hollodotme

This comment has been minimized.

Copy link
Owner

commented Feb 6, 2019

Thank you @andybee and @theseer for your feedback, much appreciated. I definitely see the point that @theseer made about hard and soft errors and fcgi endpoint independence. So I did some digging and testing.

Research

The following errors are emitted by php-fpm's main process:

I was not able to reproduce (or even find in php src) the error of premature end of script headers that @theseer mentioned. I guess this error is produced by a different client lib, like nginx maybe.

Out of interest, I was also looking for FastCGI SAPIs other than PHP that are actively maintained. So far, I was able to google the following other FastCGI SAPIs:

Conclusion

Even though there may or may not be other FastCGI SAPIs, this lib should remain independent of the SAPI. Thus introducing the ProcessManagerException for SAPI specific errors was really not the best idea from the current POV. 😞

Because there is no way to actually distinguish actual errors from the SAPI(s) and user generated errors (by error_log() and/or header() in case of php), it makes more sense to me to also let the user handle those errors and only provide sufficient information by giving access to the STDERR output separately in pass through callbacks and the response. That's why I'd like to change the implementation for v2.6.0 to:

  1. The pass through callback function declaration gets a second argument:
$callback = function( string $outputBuffer, string $errorBuffer ) {};
  1. The Response gets two new methods getOutput() (which is identical to getRawResponse() and returns the output from STDOUT) and getError() which provides the output from STDERR

  2. getRawResponse() will be deprecated in favor of consistent naming, and removed in v3.0.0.

  3. Remove the ProcessManagerException

That means users will need to check for error output and decide what to do with it theirselves.

I leave this discussion open until the end of the week for any feedback or veto before I continue with the implementation.

@andybee

This comment has been minimized.

Copy link
Author

commented Feb 6, 2019

I appreciate the detailed dive in to this and also support your proposed outcome - removing something that is specific to a single specific FastCGI server implementation feels best to me.

Maybe we could include an example that handles the PHP-FPM specific ones in a way the previous implementation did, to hint to future developers a "good" path?

@hollodotme hollodotme added this to the v2.6.0 milestone Feb 6, 2019

@theseer

This comment has been minimized.

Copy link

commented Feb 6, 2019

General

I guess no matter how we are to treat potential errors, the Steps 1-3 are sane.
It makes a lot of sense to split things up and provide the details to the user.

I guess I also agree with the assessment, that having an Exception doesn't really work given we cannot really decide whether or not status code of 400+ consitutes an error or not.

On the other hand, the current Response object does not provide any means to help that effort: As far as I can remember, the Status-Code of the server response gets lost. Unless you manually parse the raw response yourself that is.

Given that error_log() dumps to STDERR, the only way to check for a Success would be to look into what you get from getError() and run a regex against known error strings. That feels kind of cumbersome ;)

So if we do not implement error detection in the client lib - which i currently believe to be the right decision - we need to enhance the API so the user can do this in a sane fashion. At a bare minimum, this would mean to expose the response status code as an int.

In a more advanced fashion, we'd wrap that into a nice value object we could ask isSuccess() or isRedirect ... . You get the idea ;)

Example

With regards to an example as @andybee mentioned:

I like the idea but would again go a bit further and maybe conceive an opinionated FPM-Client: Everything with Status Code of 4xx or 5xx is an error (= exception) because I consider it very unlikely that any application calling scripts via fCGI would consider a 404 a useful let alone expected result.

This may very well be its own small project though.

Premature end of script headers

Yes, it's not something FPM generates but what nginx logs in case the FCGI-Server went away while processing a request - usually because it crashed. If I remember correctly, nginx translates that into a 5xx Error.

@theseer

This comment has been minimized.

Copy link

commented Feb 6, 2019

Some Additional fCGI Server implementations for other languages

@hollodotme

This comment has been minimized.

Copy link
Owner

commented Feb 6, 2019

@theseer

On the other hand, the current Response object does not provide any means to help that effort: As far as I can remember, the Status-Code of the server response gets lost. Unless you manually parse the raw response yourself that is.

You can ask for the status code using $response->getHeader('Status'), but see following...

So if we do not implement error detection in the client lib - which i currently believe to be the right decision - we need to enhance the API so the user can do this in a sane fashion. At a bare minimum, this would mean to expose the response status code as an int.

In a more advanced fashion, we'd wrap that into a nice value object we could ask isSuccess() or isRedirect ... . You get the idea ;)

The problem I see with this is, that a status code header is not always existent, except the user explicitly sets one in the script or the fCGI server produces one (in case of php-fpm if one of the errors above occur). As the FastCGI protocol does not define any status codes or even headers, this again is SAPI specific.

When calling an empty php script through php-fpm, these are the default headers sent:

X-Powered-By: PHP/7.3.1
Content-type: text/html; charset=UTF-8

(The first one surely can be supressed by ini directive expose_php=Off.)

So in general no Status: 200 OK is sent.

And even if this script would produce a fatal error or exception, there is not automatically a 5xx status header sent, nor output to STDERR. So asking

$statusCode = $response->getHeader('Status');
$error = $response->getError();

... would both result in an empty string, even though obviously something went wrong.

Thus I don't see a reliable way to provide meaningful status codes, especially in case of errors produced by the script, unless the lib relies on the fact that users will always send status codes and we set 200 OK as the default. (I don't know if I like that idea.)

Currently response headers are kept completely optional.


Premature end of script headers

Yes, it's not something FPM generates but what nginx logs in case the FCGI-Server went away while processing a request - usually because it crashed. If I remember correctly, nginx translates that into a 5xx Error.

The lib currently produces an exception, if the child process gets killed, as it leaves the underlying stream (lib <-> php-fpm) in a faulty state that is already handled.


I suggest to do the implementation as stated in my previous comment (1.-4.), do some more tests, also with other FastCGI servers, and come back here with results before the next version gets released.

hollodotme added a commit that referenced this issue Feb 10, 2019

Separate STDOUT & STDERR streams, #27
* Add second parameter to pass through callback signature for receiving
  output buffers from STDERR stream
* Inject content from STDERR stream to Response object
* Remove ProcessManagerException in case STDERR stream has content
* Replace ForbiddenException with ReadFailedException in case of an
  unexpectedly terminated stream

hollodotme added a commit that referenced this issue Feb 10, 2019

Add separate methods for STDOUT and STDERR contents, #27
* Add getOutput() as equivalent for getRawResponse()
* Add getError() as new getter for content from STDERR stream
* Deprecate getRawResponse() in favor of getOutput() / consistent naming

hollodotme added a commit that referenced this issue Feb 10, 2019

hollodotme added a commit that referenced this issue Feb 10, 2019

hollodotme added a commit that referenced this issue Mar 29, 2019

@hollodotme

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

The 4 points from here have been implemented and v2.6.0 has been released. I also wrote a short blog post about the changes, explaining the BC break for users of v2.5.0.

Though, I didn't find the time yet to check out the other fastcgi server implementations. But I'll add issues for that, so I can approach them one by one.

Closing this issue, thank you for your input and patience!

@hollodotme hollodotme closed this Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.