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

supporting HTTP/2. #399

Merged
merged 13 commits into from Jul 16, 2015

Conversation

Projects
None yet
7 participants
@kazu-yamamoto
Contributor

kazu-yamamoto commented Jul 14, 2015

This patch implements HTTP/2 (RFC 7540).

This code passes the following three tests:

-- All test cases provided by h2spec
-- Test cases provided by nghttp
-- Field test in the real web world

Recent Firefox and Chrome supports HTTP/2 over TLS. With this patch, WarpTLS automatically communicates with Firefox and Chrome in HTTP/2. There is no settings to disable HTTP/2 at this moment. We should implement them if necessary.

The following picture illustrates rough architecture:


         Input queue                          Output queue
Receiver ----------> Thread pool for WAI apps -----------> Sender

In HTTP/2, multiple streams are asynchronously sent over a TCP connection.

  • The receiver receives HTTP/2 frames, decodes them and enqueues WAI requests to the input queue. The logic of the input buffer is highly tuned.
  • A worker thread in the thread pool dequeues a WAI request, gives it to a WAI application, takes its response and enqueues the response to the output priority queue.
  • The sender packs a response in the output buffer and send it.

One concern: the number of worker threads in the worker pool is now hard coded -- 10. Unlike ResponseFile and ResponseBuilder, ResponseStream occupies the worker thread. So, if 10 streamings are served, no worker is available. We should fix this if necessary.

@snoyberg Please review and merge this if you think OK.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 14, 2015

Whoa, awesome! Will review now.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 14, 2015

I got the following when trying to test this:

Preprocessing test suite 'spec' for warp-3.1.0...

Network\Wai\Handler\Warp\HTTP2\Types.hs:25:8:
    Could not find module `Network.HTTP2.Priority'
    Perhaps you meant Network.HTTP.Proxy (from HTTP-4000.2.19)
    Use -v to see a list of the files searched for.
Completed all 8 actions.

--  While building package warp-3.1.0 using:
      C:\\Users\\Michael\\AppData\\Local\\Programs\\stack\\i386-windows\\ghc-7.8.4\\bin\\runhaskell.exe -package=Cabal-1.18.1.5 -clear-package-db -global-package-db -package-db=C:\Users\Michael\AppData\Roaming\stack\snapshots\i386-windows\lts-2.9\7.8.4\pkgdb\ C:\Users\Michael\AppData\Local\Temp\stack6928\Setup.hs --builddir=.stack-work\dist\i386-windows\Cabal-1.18.1.5\ build test:doctest test:spec
    Process exited with code: ExitFailure 1
@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 14, 2015

I can't say I understand the details going on here, but I'm OK with merging once I can compile and run the test suite on Windows.

@creichert

This comment has been minimized.

Member

creichert commented Jul 14, 2015

Very cool. I don't have anything to add but I was able to compile and
run the tests. Looking forward to testing this out more.

On Tue, Jul 14 2015, Kazu Yamamoto notifications@github.com wrote:

This patch implements HTTP/2 (RFC 7540).

This code passes the following three tests:

-- All test cases provided by h2spec
-- Test cases provided by nghttp
-- Field test in the real web world

Recent Firefox anc Chrome supports HTTP/2 over TLS. With this patch, WarpTLS automatically communicates with Firefox and Chrome in HTTP/2. There is no settings to disable HTTP/2 at this moment. We should implement them if necessary.

The following picture illustrates rough architecture:


         Input queue                          Output queue
Receiver ----------> Thread pool for WAI apps -----------> Sender

In HTTP/2, multiple streams are asynchronously sent over a TCP connection.

  • The receiver receives HTTP/2 frames, decodes them and enqueues WAI requests to the input queue. The logic of the input buffer is highly tuned.
  • A worker thread in the thread pool dequeues a WAI request, gives it to a WAI application, takes its response and enqueues the response to the output priority queue.
  • The sender packs a response in the output buffer and send it.

One concern: the number of worker threads in the worker pool is now hard coded -- 10. Unlike ResponseFile and ResponseBuilder, ResponseStream occupies the worker thread. So, if 10 streamings are served, no worker is available. We should fix this if necessary.

@snoyberg Please review and merge this if you think OK.

You can view, comment on, or merge this pull request online at:

#399

-- Commit Summary --

  • supporting HTTP/2.

-- File Changes --

M warp-tls/Network/Wai/Handler/WarpTLS.hs (64)
A warp/Network/Wai/Handler/Warp/HTTP2.hs (61)
A warp/Network/Wai/Handler/Warp/HTTP2/EncodeFrame.hs (35)
A warp/Network/Wai/Handler/Warp/HTTP2/HPACK.hs (48)
A warp/Network/Wai/Handler/Warp/HTTP2/Receiver.hs (307)
A warp/Network/Wai/Handler/Warp/HTTP2/Request.hs (141)
A warp/Network/Wai/Handler/Warp/HTTP2/Sender.hs (358)
A warp/Network/Wai/Handler/Warp/HTTP2/Types.hs (235)
A warp/Network/Wai/Handler/Warp/HTTP2/Worker.hs (106)
M warp/Network/Wai/Handler/Warp/Recv.hs (38)
M warp/Network/Wai/Handler/Warp/RequestHeader.hs (15)
M warp/Network/Wai/Handler/Warp/Response.hs (3)
M warp/Network/Wai/Handler/Warp/Run.hs (37)
M warp/Network/Wai/Handler/Warp/SendFile.hs (10)
M warp/Network/Wai/Handler/Warp/Types.hs (4)
M warp/warp.cabal (14)

-- Patch Links --

https://github.com/yesodweb/wai/pull/399.patch
https://github.com/yesodweb/wai/pull/399.diff


Reply to this email directly or view it on GitHub:
#399

connSendMany = TLS.sendData ctx . L.fromChunks
, connSendAll = sendall
, connSendFile = sendfile
, connClose = close
, connRecv = recv
, connRecv = recv ref
, connRecvBuf = recvBuf ref

This comment has been minimized.

@alexanderkjeldaas

alexanderkjeldaas Jul 14, 2015

Contributor

document the ref, recvBuf or recv functions?

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto Jul 15, 2015

Contributor

Network.Wai.Handler.Warp.Types says:

    -- | The connection receiving function. This returns "" for EOF.
    , connRecv        :: Recv
    -- | The connection receiving function. This tries to fill the buffer.
    --   This returns when the buffer is filled or reaches EOF.
    , connRecvBuf     :: RecvBuf

ref implements a cache for leftover input data.

I.writeIORef cref leftover
return ret
fill :: S.ByteString -> Buffer -> BufSize -> Recv -> IO (Bool,S.ByteString)

This comment has been minimized.

@alexanderkjeldaas

alexanderkjeldaas Jul 14, 2015

Contributor

document this function?

cont <- processStreamGuardingError $ decodeFrameHeader hd
when cont loop
processStreamGuardingError (_, FrameHeader{..})

This comment has been minimized.

@alexanderkjeldaas

alexanderkjeldaas Jul 14, 2015

Contributor

when there are multiple Context{...} and FrameHeader{..} it is a bit hard to know where streamId, continued etc are coming from. Suggest FrameHeader{streamId} and similar below.

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto Jul 15, 2015

Contributor

Thanks and done.

http2settings :: IORef Settings
, streamTable :: StreamTable
, concurrency :: IORef Int
, continued :: IORef (Maybe StreamId)

This comment has been minimized.

@alexanderkjeldaas

alexanderkjeldaas Jul 14, 2015

Contributor

This seems to indicate the stream id that the next frame should be a continuation of? Documenting this context record would make reading the code a lot easier I think.

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto Jul 15, 2015

Contributor

When a header is large, it is divided into a HEADERS frame and CONTINUATION frames. In HTTP/2, frames for several streams can be mixed. But for CONTINUATION, this is not the case. RFC 7540 says:

Other frames (from any stream) MUST NOT occur between the HEADERS
frame and any CONTINUATION frames that might follow.

This field is to implement this requirement.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 14, 2015

@snoyberg Network.HTTP2.Priority is provided in the "http2" lib. Would you add the "http2" lib in the necessary place in warp.cabal? I cannot reproduce this, so I cannot try.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 15, 2015

@snoyberg I remembered that older "http2" does not provide Network.HTTP2.Priority. So, I added the lower boundary.

@meteficha

This comment has been minimized.

Member

meteficha commented Jul 15, 2015

Awesome work, @kazu-yamamoto!

This bit worried me:

Unlike ResponseFile and ResponseBuilder, ResponseStream occupies the worker thread. So, if 10 streamings are served, no worker is available. We should fix this if necessary.

Then I read the source code: the 10 worker threads are per connection, not global :). So the only concern should be resource usage.

Perhaps parallel-io could be used to manage the thread pools? It even has a extraWorkerWhileBlocked which could be used for ResponseStreams (assuming there's some other limit to the maximum number of concurrent streams).

@meteficha

This comment has been minimized.

Member

meteficha commented Jul 15, 2015

On another thought... it doesn't look like parallel-io would fit the pattern here. It's synchronous.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 15, 2015

@meteficha Yes, 10 workers are spawn per HTTP/2 connection.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 16, 2015

@snoyberg Does the current version still fail on Windows?

@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 16, 2015

I'll test tomorrow, sorry for the delay.

On Wed, Jul 15, 2015, 5:14 PM Kazu Yamamoto notifications@github.com
wrote:

@snoyberg https://github.com/snoyberg Does the current version still
fail on Windows?


Reply to this email directly or view it on GitHub
#399 (comment).

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 16, 2015

No problem. You waited for me. I can wait for you.

kazu-yamamoto added some commits Jul 16, 2015

@alexbiehl

This comment has been minimized.

alexbiehl commented Jul 16, 2015

Windows build went without problems using stack (needed to add http2 and hex packet to extra-deps) with lts-2.9 (ghc-7.8.4).

Are there any tests which specifically target the new protocol?

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 16, 2015

Are there any tests which specifically target the new protocol?

No. I have not implemented HTTP/2 client side. So, I cannot automate testing at this moment.

Rather, I manually test the HTTP/2 feature with Firefox, h2spec and nghttp as mentioned above.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 16, 2015

Looks good on Windows (a few test failures, but nothing that doesn't exist on Windows). I think this is good to be merged, go for it when you're ready!

W00t! Congratulations Kazu, awesome work 👍

kazu-yamamoto added a commit that referenced this pull request Jul 16, 2015

@kazu-yamamoto kazu-yamamoto merged commit c5001a0 into master Jul 16, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@snoyberg snoyberg removed the in progress label Jul 16, 2015

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 16, 2015

Thanks. Merged.

Now I have an idea to spawn worker threads dynamically. I will implement it today.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 17, 2015

I changed the version of warp-tls to 3.1.0. @snoyberg Please revert this if you don't like this.

I also updated the documents of warp and warp-tls so that they explicitly mention HTTP/2.

@kazu-yamamoto kazu-yamamoto deleted the http2 branch Jul 17, 2015

@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 17, 2015

That's perfect. Should we release this to Hackage, perhaps with a caveat
announcement that this is some new behavior?

On Thu, Jul 16, 2015, 6:01 PM Kazu Yamamoto notifications@github.com
wrote:

I changed the version of warp-tls to 3.1.0. @snoyberg
https://github.com/snoyberg Please revert this if you don't like this.

I also updated the documents of warp and warp-tls so that they explicitly
mention HTTP/2.


Reply to this email directly or view it on GitHub
#399 (comment).

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 17, 2015

Let's release Warp 3.1.0 and WarpTLS 3.1.0 in the next week. I will write a blog article in advance.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Jul 17, 2015

Cool, feel free to do so yourself, or just ping me and I'll do the release.

On Thu, Jul 16, 2015 at 6:10 PM Kazu Yamamoto notifications@github.com
wrote:

Let's release Warp 3.1.0 and WarpTLS 3.1.0 in the next week. I will write
a blog article in advance.


Reply to this email directly or view it on GitHub
#399 (comment).

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 17, 2015

Now I have an idea to spawn worker threads dynamically. I will implement it today.

Done in fedab88.

@creichert

This comment has been minimized.

Member

creichert commented Jul 28, 2015

@kazu-yamamoto Do you know exactly which sha1 was uploaded to Hackage as Warp-3.1? Was it 70fa8a6 ? I want to tag it for helping some testing in the servant repository.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 29, 2015

@creichert Sorry. I forgot to push tags. I did now.

@creichert

This comment has been minimized.

Member

creichert commented Jul 29, 2015

Thanks!

On Tue, Jul 28 2015, Kazu Yamamoto notifications@github.com wrote:

@creichert Sorry. I forgot to push tags. I did now.


Reply to this email directly or view it on GitHub:
#399 (comment)

@yairchu

This comment has been minimized.

yairchu commented on warp/Network/Wai/Handler/Warp/Run.hs in 562371a Jan 22, 2018

Isn't the S.length bs0 >= 4 part redundant?? (that is always true when the second condition is true)
Does this speed things up or something?

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