Pim-community-dev: API: Response contains unexpected HTML for Create/Update multiple attributes

[pardahlman]

Hello,

I'm running Akeneo PIM CE version 1.7.2, here's my current system information: akeneo-pim-system-info.txt.

I'm trying out the API endpoints for creating/updating several attributes.

The request is accepted by the API, and I can see the added/updated attributes in the web UI. The response comes back with a 200 OK, but the content in the response does not look like what I'm expecting:

{"line":1,"code":"test_5","status_code":204}<br />
<b>Notice</b>:  ob_flush(): failed to flush buffer. No buffer to flush in <b>/akeneo/vendor/akeneo/pim-community-dev/src/Pim/Bundle/ApiBundle/Stream/StreamResourceResponse.php</b> on line <b>248</b><br />

{"line":2,"code":"test_6","status_code":204}<br />
<b>Notice</b>:  ob_flush(): failed to flush buffer. No buffer to flush in <b>/akeneo/vendor/akeneo/pim-community-dev/src/Pim/Bundle/ApiBundle/Stream/StreamResourceResponse.php</b> on line <b>248</b><br />

If I'm reading the documentation correctly, the response for this particular case would be

{"line":1,"code":"test_5","status_code":204}
{"line":2,"code":"test_6","status_code":204}

I haven' tried out the corresponding endpoints for the other resources (Product, Family etc). I'll be writing a custom parser for the response, for now - just thought I'd report this issue.

Keep up the good work 👍

[pardahlman]

This is a bit of topic... but in my opinion, this issue demonstrates why custom media types and formats are problematic. It adds complexity to the caller, as it can't use any serializer out-of-the-box, needs to care about media type headers etc. It also adds complexity to the server side, which I guess is the reason this bug exists.

If I would implement a similar endpoint, I would just communicate with lists/arrays of resources in plain json (which is basically what is done now, expect there are no array start array end tokens ([ ]) and instead of comma, there is a new line).

I'd be interested to hear what you think!

[ahocquard]

Hello @pardahlman, could you tell me the value of the parameter "output_buffering" defined in your php.ini ?

[pardahlman]

@ahocquard - thanks for getting back to me! It turns out that output_buffering is not set at all in the ini file at all(!). If I understand correctly, that is equivalent to the value off.

[ahocquard]

@pardahlman Yes,you should configure it with a value (4096 for example). FYI, there are two buffers : ob_flush buffer and flush buffer.

ob_flush is not automatically started in CLI, but it should be with Apache (with the parameter output_buffering).

Flush is the internal buffer of php, and then, you cannot configure it.

But it is a real issue and I will discuss with the team the fix we should apply :

• update the documentation to indicate that you should use output_buffering
• or modify the stream response to handle a such case where the parameter is set to Off

About your second message :

• stream response could handle (in theory) a lot of products, one by one, without loading everything in memory. The main drawback with the JSON is that you have to get the whole content in memory before processing the data (or you need to use a JSON parser on the fly).

For example, with a clever client, you could consume the response body line by line.

[pardahlman]

Hi @ahocquard,

I think both your points for the fix sounds like a good idea 👌 . I'll update the php.ini according to your suggestions next time I build the docker container.

Also, it makes sense to work with response streams. Thanks for clarifying! I wonder, though... wouldn't you be able to use a stream and still return application/json? Anyways, I hear what you're saying!

[ahocquard]

Hi @pardahlman

I see two meanings by your words "you be able to use a stream and still return application/json".

If it means "be able to stream a response and having a JSON body", yes it's possible but it's useless because you load everything in memory (in a php array), then json_encode it, then stream it.

And if you build a JSON array on the fly, you are finally doing the same thing as we are doing, but with extra stuff for the client. Reading line by line a stream is (pretty) easy. Reading element by element a json array without decoding the whole content at first is generally out of scope of the main json parsers.

If it means "use application/json even it's not really a json but a json per line", then it would be an error because a serializer could try to automatically decode it depending of the content-type, and it would raise an error (because it's not a real json).

[pardahlman]

I'm obviously not an expert in how to do things in PHP, but I would expect that you could create a json body by simple starting you response stream with [ and then inject , at the end of each line and the add ] before closing the steam. That is perhaps difficult to do.

[QuingKhaos]

Same here, platform.sh default is output_buffer = 0, so downloading every media failed because StreamedFileResponse doesn't check if an output buffer exists.

IMHO if you want to flush an output buffer because you want to stream it, you must check first if there is an output buffer which can be flushed.

[ahocquard]

@patkar thanks for your report. It will be fixed in a patch.

[mickaelandrieu]

Hello and thank you (again) for your feedbacks @pardahlman!

@ahocquard to sum up this thread, the issue can be fixed by setting up a non false value to output_buffering property in php.ini file?

@patkar if you use platform.sh to deploy Akeneo projects, please contact me, I'm interested by your feedbacks :)

[ahocquard]

@mickaelandrieu To sum-up, I think that ouptput_buffer should not be used, looking at BinaryFileResponse implementation of Symfony => problem resolved ;)

[QuingKhaos]

@mickaelandrieu You want to flush output buffers with chunks because you want to stream the response. That's totally ok. But you don't check if there is an existing output buffer before you flush it, so it fails as long as automatic outbut_buffering in the php.ini is false.

You need to check with ob_get_status()/ob_get_level() if there is an output buffer (and how many) before calling ob_flush() here. Better you simply close all outbut buffers instead. Echoing only one chunk and directly flushing is IMHO equivalent to not using output buffering. E.g.

use Symfony\Component\HttpFoundation\Response;

// ...

        $callback = function () use ($resource) {
            // close all existing output buffers (if there are any) and clean
            // the output so far, instead of outputting warnings
            Response::closeOutputBuffers(0, false);

            while (!feof($resource)) {
                $buffer = fread($resource, StreamedFileResponse::CHUNK);
                echo $buffer;

                // the flush() is still needed and has nothing to do with output buffering
                // it's there to flush output buffers of the SAPI (fpm or mod_php buffers)
                flush();
            }
            fclose($resource);
        };

P.S: Always use flush() with ob_flush() together ;)

[ahocquard]

@patkar Actually, I think we should not use ob_flush at all. It is in the Symfony documentation only for the example.
Internally, internal output buffering (=ob_flush) has already a buffer (4k by default). Also, PHP has an internal buffer (=flush). So, each chunk of 4k is flushed.

Apache has an internal buffer as well (8k by default I believe). Your client (a browser for example) can have a buffer when receiving the response body.

So, even if you are forcing the flush of the data in the PHP process, a layer will probably bufferise it later. An your PHP process is already handling the buffer for you if output_buffer is on.

[QuingKhaos]

I think we should not use ob_flush at all.

That's the reason, why I recommend to close all output buffers instead with Response::closeOutputBuffers(0, false);

Internally, internal output buffering (=ob_flush) has already a buffer (4k by default).

It's not internal, it's called default output buffering. But there are system which have default output_buffer set to off in the php.ini, like platform.sh. That's the paintpoint of this issue, calling ob_flush() without an active output buffer fails -> generates a notice and destroys API output and images on the FileController::donwloadAction().

Also, PHP has an internal buffer (=flush). So, each chunk of 4k is flushed.

The SAPI implementation (fpm, mod_php) has ;). That's the reason, why still should call flush() (together with ob_flush() if output buffer would be active). 4k is to big a streaming each response immediately.

Apache has an internal buffer as well (8k by default I believe). [..] So, even if you are forcing the flush of the data in the PHP process, a layer will probably bufferise it later.

That's something which can be tricky, yes. Good article about streaming PHP responses with webserver config tips: https://www.jeffgeerling.com/blog/2016/streaming-php-disabling-output-buffering-php-apache-nginx-and-varnish

And your PHP process is already handling the buffer for you if output_buffer is on.

But a streaming response doesn't want PHP to handle output buffering. And PHP won't if output_buffer set to off in the php.ini. Which is the root cause of this issue and triggering the bug in the code.

[ahocquard]

Internally, internal output buffering (=ob_flush) has already a buffer (4k by default).

I say internal buffer because you can open it in your own php process. And it can be automatically opened, depending on the parameter output_buffering in the php.ini.

4k is to big a streaming each response immediately.

That's subjective it depends a lot of you data ;) It avoids a lot network IO (and that's the main reason of the buffer).

Symfony don't use flush and ob_flush for binary file response: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/BinaryFileResponse.php#L302

Their solution works, even if you use ob_flush in another class before.

But a streaming response doesn't want PHP to handle output buffering.

The responsibility of your response is to stream data in the standard output. The responsibility of the buffer could be handle by you process, your webserver etc.

[QuingKhaos]

That's subjective it depends a lot of you data ;) It avoids a lot network IO (and that's the main reason of the buffer).

Yes, but the point of this API response format is got get exactly each item by item. not chopped by some output buffer chunks -> thus, don't use output buffering -> don't depend on any defaults you can't control.

Symfony don't use flush and ob_flush for binary file response:

Yes, but it's a file response. So automatic buffering doesn't harm. And it does a copy_to_stream() so you can't chunk the response either.

[LaureBrosseau]

Hi @pardahlman @patkar,
this issue will be fixed in the 1.7.5, we'll let you know once it's released.
Thanks!

[mickaelandrieu]

Hello @pardahlman , @patkar ,

the 1.7.5 just got released :)

Thanks for report!