Re: 100 Continue header fix in CURL http adaptor. #51

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@gregnacu

Hi!

I've forked sag, copied in my fix and commited it to my fork.

I'm not super familiar with Git, I spend all my time on private corporate SVN repositories. But, I've heard good things about Git so let's give this whirl.

Greg.

@sbisbee
Owner
sbisbee commented Jul 20, 2012

Hey Greg,

Thanks again for sending this code along. On a hunch I just did some Google-ing and have some follow up questions that might make this code not needed...

  • Are you interfacing with CouchDB directly or through a reverse proxy like Apache?
  • What version of PHP and cURL are you using? I have a feeling that cURL is adding a Expect: 100-continue header.

Would like to avoid the extra code/logic here if possible.

Cheers.

@gregnacu

We're using 5.3.10. PHP info reports curl as:

curl

cURL support => enabled
cURL Information => 7.21.4

And, we're using cloudant, so I would imagine that cloudant is using a reverse proxy, I don't know if it's apache though.

The thing is, the SAG cURL http adaptor class already had code that was trying to handle the presence of the 100 continue header, but, it was handling it wrong.

However, one of my developers is reading some curl forums and thinks that there's a way to specify for it to not add that header in. 100 Continue is part of HTTP, though, and the original implementation that was in your code was not handling it properly.

Are you concerned that my implementation of a fix won't be fast enough?

Greg.

On 2012-07-20, at 4:38 PM, Sam Bisbee wrote:

Hey Greg,

Thanks again for sending this code along. On a hunch I just did some Google-ing and have some follow up questions that might make this code not needed...

  • Are you interfacing with CouchDB directly or through a reverse proxy like Apache?
  • What version of PHP and cURL are you using? I have a feeling that cURL is adding a Expect: 100-continue header.

Would like to avoid the extra code/logic here if possible.

Cheers.


Reply to this email directly or view it on GitHub:
#51 (comment)

@sbisbee
Owner
sbisbee commented Jul 23, 2012

Luckily I work at Cloudant, so I know what we're doing. :) Also, weird that the Continue code in there isn't working for you - it's worked for others.

Short story, I think that this is happening: http://the-stickman.com/web-development/php-and-curl-disabling-100-continue-header/

Try this (quick hack, assumes no one is defining Expect headers anywhere else).

diff --git a/src/httpAdapters/SagCURLHTTPAdapter.php b/src/httpAdapters/SagCURLHTTPAdapter.php
index f631a5a..80d1e6b 100644
--- a/src/httpAdapters/SagCURLHTTPAdapter.php
+++ b/src/httpAdapters/SagCURLHTTPAdapter.php
@@ -42,6 +42,8 @@ class SagCURLHTTPAdapter extends SagHTTPAdapter {
       foreach($headers as $k => $v) {
         $opts[CURLOPT_HTTPHEADER][] = "$k: $v";
       }
+
+      $opts[CURLOPT_HTTPHEADER][] = "Expect:";
     }

     // send data through cURL's poorly named opt
@gregnacu

Hmm. Yeah, I see that. And of course, we ARE using cURL, and it is very likely cURL that's contributing to the problem, however, is this a bug in cURL?

Furthermore, regardless of whether it's a bug in cURL for PHP, I simply cannot see how this code works:

// split headers and body
list($continue, $headers, $response->body) = explode("\r\n\r\n", $chResponse);

  /*
   * It doesn't always happen, but it seems that we will sometimes get a
   * Continue header that will screw parsing up.
   */
  if(!$response->body) {
    $response->body = $headers;
    $headers = $continue;
  }

  unset($continue);

This is the code that I replaced. The problem is that if the body itself contains \r\n\r\n, which it totally can, because couch returns that from a variety of requests, then $continue gets set to the block of standard headers, $headers gets set to only the first part of the body, and $response->body gets set to the second part of the body, and it's possible that there is a third, fourth of more part of the body that is not captured at all. Then the if fails, $continue gets unset... but... the $response->body clearly does not contain the full body! I'm not sure how other people are not experiencing this as a problem.

Am I crazy?
Greg.

On 2012-07-23, at 11:53 AM, Sam Bisbee wrote:

Luckily I work at Cloudant, so I know what we're doing. :) Also, weird that the Continue code in there isn't working for you - it's worked for others.

Short story, I think that this is happening: http://the-stickman.com/web-development/php-and-curl-disabling-100-continue-header/

Try this (quick hack, assumes no one is defining Expect headers anywhere else).

diff --git a/src/httpAdapters/SagCURLHTTPAdapter.php b/src/httpAdapters/SagCURLHTTPAdapter.php
index f631a5a..80d1e6b 100644
--- a/src/httpAdapters/SagCURLHTTPAdapter.php
+++ b/src/httpAdapters/SagCURLHTTPAdapter.php
@@ -42,6 +42,8 @@ class SagCURLHTTPAdapter extends SagHTTPAdapter {
      foreach($headers as $k => $v) {
        $opts[CURLOPT_HTTPHEADER][] = "$k: $v";
      }
+
+      $opts[CURLOPT_HTTPHEADER][] = "Expect:";
    }

    // send data through cURL's poorly named opt

Reply to this email directly or view it on GitHub:
#51 (comment)

@sbisbee
Owner
sbisbee commented Jul 31, 2012

You are not crazy. :) The people testing that code - I never ran into this problem myself, so I couldn't test it - must not have been hitting endpoints that streamed or had non-JSON attachments. I'm going to pull the code out since it isn't even needed anymore.

Technically an HTTP/1.1 origin server could still send Sag a Continue header, but Couch and Cloudant don't, so we're fine. Assuming someone doesn't use some proxy software that likes to inject things on its own. shrug We can cross that bridge if we come to it.

Anyways, thanks again for working with me to debug this! Also, I look forward to any other contributions from you. I'm trying to put together a plan for v1.0 now.

Cheers.

@sbisbee sbisbee closed this Jul 31, 2012
@mglcel
mglcel commented Feb 22, 2013

Hi, a solution posted in our repo seems shorter :
https://github.com/viadeo/api-sdk-php/pull/2/files

Thanks to @thomas-klein

 674    $result_sections = explode("\r\n\r\n", $result);
 675    $body = end($result_sections);
 676    $header = prev($result_sections);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment