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

Inputs using HTTP transport do not support Expect: 100-continue #1939

Closed
mikkolehtisalo opened this Issue Mar 16, 2016 · 0 comments

Comments

Projects
None yet
2 participants
@mikkolehtisalo
Contributor

mikkolehtisalo commented Mar 16, 2016

Problem description

If a HTTP GELF client fragments the message using Expect: 100-continue into separate header and body parts the server fails to handle message correctly.

Steps to reproduce the problem

  1. Use a client that is affected by default, for example the .NET HttpWebRequest
  2. POST HTTP GELF using http version 1.1
  3. Client receives 202 Accepted, thinks that everything went ok
  4. Server throws IllegalStateException("GELF message is too short. Not even the type header would fit.") at GelfMessage.java
  5. Message is lost

Network traffic

Looking at the network traffic reveals (ordinary syn/ack stuff omitted):

Client (one packet):

POST /gelf HTTP/1.1
User-Agent: User agent x
Content-Type: text/json
Host: target:port
Content-Length: xxxx
Expect: 100-continue
Accept-Encoding: gzip
Connection: Close

Server (one packet):

HTTP/1.1 202 Accepted
Content-Length: 0
Connection: close

Client(One or more packets):

GELF/JSON BODY

Server:

Huh?!

Options

  1. Document that fragmentation must not be used. For example .NET HttpWebRequest requires setting Expect100Continue=false using ServicePointManager.
  2. Implement missing feature to HTTP transports.
  3. Do not return 202 Accepted if body size == 0, return 4xx or 5xx instead, with a "body was empty" error message in body

@kroepke kroepke added bug S2 P3 labels Mar 18, 2016

@kroepke kroepke added this to the 2.0.0 milestone Mar 18, 2016

kroepke added a commit that referenced this issue Mar 30, 2016

Add Expect: 100-Continue support to HTTP input
Previously the input blindly answered with 202 Accepted but instead it should properly support the Expect header.
Failing to respond correctly resulted in loss of messages because the client's expectation was broken.

Adding a HttpChunkAggregator fixes the issue while still allowing non-chunked requests to be sent.

fix #1939

@kroepke kroepke added the triaged label Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment