Opened 4 years ago

Closed 3 years ago

#4673 defect closed fixed (fixed)

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

Reported by: itamar Owned by: itamar
Priority: low Milestone:
Component: web Keywords:
Cc: msteder Branch: branches/100-continue-4673
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

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 http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3 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 4 years ago by itamar

  • Component changed from core to web
  • Owner changed from glyph to jknight

comment:2 Changed 3 years ago by <automation>

  • Owner jknight deleted

comment:3 Changed 3 years ago by msteder

  • Cc msteder added

comment:4 Changed 3 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/100-continue-4673

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

comment:5 Changed 3 years ago by itamar

  • 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 3 years ago by itamar

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/100-continue-4673 is the buildbot run, just started, hopefully they all pass :).

comment:7 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to itamar
  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 3 years ago by itamar

  • Keywords review added
  • Owner itamar 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: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/100-continue-4673

comment:9 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to itamar

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 3 years ago by itamar

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

comment:11 Changed 3 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(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.