Opened 11 years ago

Closed 10 years ago

#4673 defect closed fixed (fixed)

The twisted.web server should not ignore "Expect: 100-continue"

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: low Milestone:
Component: web Keywords:
Cc: msteder Branch: branches/100-continue-4673
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst


From the RFC: "The purpose of the 100 (Continue) status ... is to allow a client that is sending a request message with a request body to determine if the origin server is willing to accept the request (based on the request headers) before the client sends the request body."

See for details.

Supporting the "Expect: 100-continue" header on an API level is currently difficult, since twisted.web always reads the full request body before handing the request to business logic code. Until we can allow custom Resources to handle requests before the body is read, we should at the very least return the appropriate "100 (Continue)" response to the HTTP client so it can continue.

Change History (11)

comment:1 Changed 11 years ago by Itamar Turner-Trauring

Component: coreweb
Owner: changed from Glyph to jknight

comment:2 Changed 11 years ago by <automation>

Owner: jknight deleted

comment:3 Changed 10 years ago by msteder

Cc: msteder added

comment:4 Changed 10 years ago by itamarst

Author: itamarst
Branch: branches/100-continue-4673

(In [32482]) Branching to '100-continue-4673'

comment:5 Changed 10 years ago by Itamar Turner-Trauring

Keywords: review added

This is an implementation of the simple, just respond with 100 continue, suggestion. A user-exposed API depends on #288. I tested with curl, and without this POSTs take an extra 1 second as curl waits for the 100 continue, decides it's not going to happen and then switches to normal processing.

comment:6 Changed 10 years ago by Itamar Turner-Trauring is the buildbot run, just started, hopefully they all pass :).

comment:7 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Itamar Turner-Trauring
  1. I think that writing out a 100-Continue response as soon as the "Expect: 100-Continue" request header is received has a minor problem. A later request header may be unparseable and result in a 400 response; for example, a multipart/form-data request without a content-disposition field results in a 400 response. Another case which may result in a 400 response is the case where the maximum number of request headers is exceeded. It's not the end of the world if we send a 100 and then a 400, probably, but it would probably be better to avoid doing so.
  2. It's not immediately obvious that this works correctly with pipelined requests. I think it does, but it would be nice to see a unit test demonstrating that the response to an earlier request isn't corrupted by a later request which includes an "Expect: 100-Continue".
  3. test_HTTP10 docstring typos Except for Expect

comment:8 Changed 10 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring deleted

All 3 comments were addressed. As a result of the change, the Expect header will now (as in trunk) be exposed to user code; this can be changed if necessary.

Test run, in progress:

comment:9 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Itamar Turner-Trauring

Thanks, I like the look of this. One thing though, for the behavior exercised by the new test_expect100ContinueWithPipelining, I was thinking of the case where requests arrive in the opposite order as they do in the current implementation, and where the first request has an asynchronous response which lasts past the time when the second request is received. If you feel like adding that one, cool, otherwise I don't know if the new one that's there now offers much - I suppose it's a another case but it doesn't seem like one that's very likely to different.

Also, the news fragment says Twisted.web's server code but it should probably be twisted.web's server code or even twisted.web.server.

Address to your satisfaction and merge. Thanks.

comment:10 Changed 10 years ago by Itamar Turner-Trauring

I'm just going to merge as is (with updated news file).

comment:11 Changed 10 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [32596]) Merge 100-continue-4673.

Fixes: #4673 Author: itamarst Review: exarkun

twisted.web server code now responds to 100-continue headers, although this is not controllable by user code yet.

Note: See TracTickets for help on using tickets.