add support for 100-continue in HTTP::Server::Response #6912

repomaa · 2018-10-06T11:54:51Z

No description provided.

src/http/server/response.cr

straight-shoota · 2018-10-08T10:38:46Z

Go's webserver implementation doesn't provide a means to send 100 Continue explicitly, but it automatically sends the continue header when Expect: 100-continue is present as soon as the handler reads from the IO.
See https://golang.org/pkg/net/http/#ResponseWriter

Copying this might even be more useful since it just works with any handler and doesn't require any custom logic.

repomaa · 2018-10-09T11:34:25Z

@straight-shoota That is perfect!

src/http/common.cr

straight-shoota · 2018-10-09T18:04:13Z

I noticed there is an issue when the handler doesn't read from the IO and 100 Continue status should not be sent. A spec for this case is missing. But Content#close calls skip_to_end which read the entire IO and thus sent 100 Continue nonetheless. I suppose #close just needs to set @expects_continue = false before calling skip_to_end.

straight-shoota · 2018-10-09T18:10:07Z

Actually, I'm not entirely sure why skip_to_end even is there... It seems to be the only reason for the Content type in the first place.

But I can't see how that is actually useful, and at least, it is confusing to the user. It doesn't just close the IO in it's current state but completely consumes it until the sender is done (maybe a happy curl -T /dev/urandom example.com? 😆 ).
I mean you don't have to call it, just exiting the handler will close the socket without consuming everything.
I'm just wondering what's the use case and if there is a better solution?

repomaa · 2018-10-09T18:21:15Z

@straight-shoota i agree. this is weird indeed. But I think it should be a seperate issue.

spec/std/http/server/server_spec.cr

Sija · 2018-10-09T18:28:26Z

src/http/content.cr

      skip_to_end
      super
    end
+
+    def expects_continue=(value : Bool)


Perhaps cleaner: property? expects_continue : Bool

src/http/content.cr

RX14 · 2018-10-10T16:49:48Z

src/http/content.cr

      skip_to_end
      super
    end
+
+    protected def ensure_send_continue
+      return unless @expects_continue && !@continue_sent


return unless @expects_continue return if @continue_sent

straight-shoota · 2018-10-22T21:16:38Z

spec/std/http/server/server_spec.cr

+      address = server.bind_unused_port
+      spawn { server.listen }
+
+      Fiber.yield


Please adjust this according to #6953

RX14 · 2018-10-24T11:18:39Z

@jreinert needs a format

repomaa · 2018-10-24T20:16:02Z

@jreinert needs a format

@RX14 done

src/http/content.cr

RX14

Just one last thing.

Sija · 2018-10-28T12:23:19Z

src/http/content.cr

      @io.peek
    end

    def skip(bytes_count)
+    enensure_send_continue


uh jezzus.. i should sleep more. xD

Sija · 2018-10-28T12:46:00Z

src/http/content.cr

      @io.peek
    end

    def skip(bytes_count)
+      enensure_send_continue


One en too much.

... i don't... even..

Relax, that happens, especially when there's lack of sleep involved :) I'd just advise "proofreading" your PRs before submitting ;)

repomaa force-pushed the 100-continue branch 3 times, most recently from 23add65 to 76dc76c Compare October 6, 2018 12:09

straight-shoota reviewed Oct 6, 2018

View reviewed changes

src/http/server/response.cr Outdated Show resolved Hide resolved

repomaa force-pushed the 100-continue branch 3 times, most recently from 904b7b4 to e453c1a Compare October 9, 2018 17:33

straight-shoota reviewed Oct 9, 2018

View reviewed changes

src/http/common.cr Show resolved Hide resolved

src/http/common.cr Outdated Show resolved Hide resolved

repomaa force-pushed the 100-continue branch from e453c1a to 9508f23 Compare October 9, 2018 18:25

Sija reviewed Oct 9, 2018

View reviewed changes

spec/std/http/server/server_spec.cr Show resolved Hide resolved

Sija reviewed Oct 9, 2018

View reviewed changes

repomaa force-pushed the 100-continue branch from 9508f23 to 3c68c23 Compare October 9, 2018 18:30

Sija reviewed Oct 9, 2018

View reviewed changes

src/http/content.cr Show resolved Hide resolved

repomaa force-pushed the 100-continue branch from 3c68c23 to 186d3b6 Compare October 10, 2018 09:34

straight-shoota mentioned this pull request Oct 10, 2018

skip_to_end in HTTP::Content#close: Useful or problematic? #6924

Closed

RX14 requested changes Oct 10, 2018

View reviewed changes

repomaa force-pushed the 100-continue branch from 186d3b6 to 218f0f4 Compare October 22, 2018 21:00

straight-shoota reviewed Oct 22, 2018

View reviewed changes

repomaa force-pushed the 100-continue branch from 218f0f4 to ac7ef95 Compare October 23, 2018 17:39

repomaa force-pushed the 100-continue branch from ac7ef95 to 99bb9b2 Compare October 24, 2018 20:15

straight-shoota approved these changes Oct 24, 2018

View reviewed changes

RX14 reviewed Oct 25, 2018

View reviewed changes

src/http/content.cr Show resolved Hide resolved

RX14 requested changes Oct 25, 2018

View reviewed changes

repomaa force-pushed the 100-continue branch from 99bb9b2 to 8074283 Compare October 28, 2018 12:10

Sija suggested changes Oct 28, 2018

View reviewed changes

repomaa force-pushed the 100-continue branch 3 times, most recently from 4b55742 to 670224d Compare October 28, 2018 12:38

Sija suggested changes Oct 28, 2018

View reviewed changes

add Expect: 100-continue header handling in HTTP::Server::Response

cef040c

repomaa force-pushed the 100-continue branch from 670224d to cef040c Compare October 28, 2018 12:55

RX14 approved these changes Oct 28, 2018

View reviewed changes

straight-shoota approved these changes Nov 6, 2018

View reviewed changes

RX14 requested a review from bcardiff November 7, 2018 16:10

bcardiff approved these changes Nov 7, 2018

View reviewed changes

bcardiff added kind:feature topic:stdlib labels Nov 7, 2018

RX14 merged commit 735b0be into crystal-lang:master Nov 9, 2018

RX14 added this to the 0.27.1 milestone Nov 9, 2018

bcardiff added topic:stdlib:networking and removed topic:stdlib labels Jan 26, 2019

ghost mentioned this pull request Jun 7, 2019

Missing support for 100-Continue in HTTP::Client #7866

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for 100-continue in HTTP::Server::Response #6912

add support for 100-continue in HTTP::Server::Response #6912

repomaa commented Oct 6, 2018

straight-shoota commented Oct 8, 2018

repomaa commented Oct 9, 2018

straight-shoota commented Oct 9, 2018

straight-shoota commented Oct 9, 2018

repomaa commented Oct 9, 2018

Sija Oct 9, 2018

repomaa Oct 9, 2018

RX14 Oct 10, 2018

straight-shoota Oct 22, 2018

RX14 commented Oct 24, 2018

repomaa commented Oct 24, 2018 •

edited

Loading

RX14 left a comment

Sija Oct 28, 2018

repomaa Oct 28, 2018

Sija Oct 28, 2018

repomaa Oct 28, 2018

Sija Oct 28, 2018 •

edited

Loading

add support for 100-continue in HTTP::Server::Response #6912

add support for 100-continue in HTTP::Server::Response #6912

Conversation

repomaa commented Oct 6, 2018

straight-shoota commented Oct 8, 2018

repomaa commented Oct 9, 2018

straight-shoota commented Oct 9, 2018

straight-shoota commented Oct 9, 2018

repomaa commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Oct 24, 2018

repomaa commented Oct 24, 2018 • edited Loading

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Oct 28, 2018 • edited Loading

Choose a reason for hiding this comment

repomaa commented Oct 24, 2018 •

edited

Loading

Sija Oct 28, 2018 •

edited

Loading