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

Can Alternator correctly handle HTTP requests that include the "Expect: 100-continue" HTTP request header? #6844

Closed
Felix-zhoux opened this issue Jul 15, 2020 · 9 comments
Assignees
Labels
area/alternator Alternator related Issues type/bug
Milestone

Comments

@Felix-zhoux
Copy link

Felix-zhoux commented Jul 15, 2020

Installation details
Scylla version (or git commit hash): 4.0.0-0.20200601.d95aa77b6
Cluster size: 1
OS (RHEL/CentOS/Ubuntu/AWS AMI): Centos

When I use Dynamo SDK to send a request to Scylla Alternator, if I send the request directly to Alternator, I will get a delay of more than 1s. If I send it to apisix first, and then apisix forwards it to Alternator, I can get a normal ms level. delay.

After tcpdump debug, I found that the delay of 1s comes from the stage of establishing TCP connection and sending request data. The Alternator delay monitoring in the Scylla monitor also verifies this. The delay of the Alternator processing PutItem and GetItem operations is still ms level, but the dynamo SDK client gets a delay of about 1s.

After apisix, you can get a normal delay, because apisix can normally handle HTTP requests with the "Expect: 100-continue" header, and the requests forwarded to the Alternator no longer have the "Expect: 100-continue" header.
Attached below are some screenshots of my debug process.
I’m not sure if my guess is correct, I need your help to verify

HTTP request sent directly to ScyllaAlternator
The client gets a delay of about 1s
image

image

HTTP request is forwarded to ScyllaAlternator via apisix routing
The client gets a normal ms-level delay
image

image

image

ps: Dynamo SDK client(10.10.10.35), apisix(10.10.10.92), Scylla(10.10.10.31)

@nyh nyh added the area/alternator Alternator related Issues label Jul 15, 2020
@nyh nyh self-assigned this Jul 15, 2020
@nyh
Copy link
Contributor

nyh commented Jul 15, 2020

Nice detective work!
I haven't seen these 1 second delays in anu of my tests (what is this "Dynamo SDK" - which language is it?), but you are right - the "Expect: 100-continue" is a part of the HTTP 1.1 RFC (see https://tools.ietf.org/html/rfc7231#section-5.1.1) and Seastar's HTTP server is missing support for it :-( I will have to add it.

@nyh nyh added the type/bug label Jul 15, 2020
@Felix-zhoux
Copy link
Author

I haven't seen these 1 second delays in anu of my tests (what is this "Dynamo SDK" - which language is it?)

aws-cpp-sdk-dynamodb (Sorry, that's my spoken language. -_-!)

@psarna
Copy link
Contributor

psarna commented Jul 15, 2020

By the way, AWS C++ SDK documentation mentions a quite easy workaround (source: https://docs.aws.amazon.com/sdk-for-cpp/v1/developer-guide/client-config.html):

disableExpectHeader

  • Applicable only for CURL HTTP clients. By default, CURL adds an “Expect: 100-Continue” header in an HTTP request to avoid sending the HTTP payload in situations where the server responds with an error immediately after receiving the header. This behavior can save a round-trip and is useful in situations where the payload is small and network latency is relevant. The variable’s default setting is false. If set to true, CURL is instructed to send both the HTTP request header and body payload together.

@nyh
Copy link
Contributor

nyh commented Jul 15, 2020

This will need to be fixed in the Seastar library, where the HTTP server code that we use lives. I opened an issue there: scylladb/seastar#768

@nyh
Copy link
Contributor

nyh commented Jul 15, 2020

I haven't seen these 1 second delays in anu of my tests (what is this "Dynamo SDK" - which language is it?)

aws-cpp-sdk-dynamodb (Sorry, that's my spoken language. -_-!)

My question was about the programming language of that SDK (C++? Java? Python? Go?), not your spoken language :-)
So the answer is C++. Thanks. Surprisingly, I think that's the only language that we haven't experimented with - we test heavily with the Python SDK, and experimented less heavily with the Java, Go, and Node.JS SDKs. So it's good that you are testing it.

@slivne slivne added this to the 4.3 milestone Jul 16, 2020
@nyh nyh assigned wmitros and unassigned nyh Aug 6, 2020
@avikivity
Copy link
Member

@nyh please evaluate for backport. It doesn't look like a regression, but if it makes alternator unusable, it's a major bug.

@nyh
Copy link
Contributor

nyh commented Aug 10, 2020

Yes, it does make Alternator unusable for users of a specific driver (a C++ one) which appears to use this HTTP feature. However, I'm not sure how to backport. I guess that instead of putting a new version of Seastar in the branches, I should backport the specific Seastar patch to the branch-specific-seastar we have? So I should basically follow the instructions for that in the maintainers.md?

@avikivity
Copy link
Member

I suppose we need an instructions.md that tells you to follow the instructions.

nyh pushed a commit to scylladb/scylla-seastar that referenced this issue Aug 11, 2020
This patch enables sending a response with status code "100 Continue"
before reading the requests body if the request contains
a "Expect: 100-continue" header.

Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>
(cherry picked from commit 994a1ce)
Refs scylladb/scylladb#6844
nyh added a commit that referenced this issue Aug 11, 2020
  > http: add "Expect: 100-continue" handling

Refs #6844.
nyh pushed a commit to scylladb/scylla-seastar that referenced this issue Aug 11, 2020
This patch enables sending a response with status code "100 Continue"
before reading the requests body if the request contains
a "Expect: 100-continue" header.

Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>
(cherry picked from commit 994a1ce)
Refs scylladb/scylladb#6844
nyh added a commit that referenced this issue Aug 11, 2020
  > http: add "Expect: 100-continue" handling

Fixes #6844
nyh pushed a commit to scylladb/scylla-seastar that referenced this issue Aug 11, 2020
This patch enables sending a response with status code "100 Continue"
before reading the requests body if the request contains
a "Expect: 100-continue" header.

Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>
(cherry picked from commit 994a1ce)
Refs scylladb/scylladb#6844
nyh added a commit that referenced this issue Aug 11, 2020
  > http: add "Expect: 100-continue" handling

Fixes #6844
@nyh
Copy link
Contributor

nyh commented Aug 11, 2020

Backported the Seastar patch to the Seastar branch of Scylla 4.2, 4.1 and 4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alternator Alternator related Issues type/bug
Projects
None yet
Development

No branches or pull requests

7 participants