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

Guarantee atomicity of WINDOW_UPDATE increments. #622

Merged
merged 2 commits into from May 30, 2017

Conversation

Projects
None yet
2 participants
@lucasdicioccio
Contributor

lucasdicioccio commented May 25, 2017

The current code may lead to over-estimating the window size.
The over-estimation occurs as follows:

  • the initial window state is W
  • the receiver observes a window update and computes W+K in a first
    transaction
  • the sender decrease the window by the payload size P, hence the window
    state is set to W-P
  • the receiver writes W+K in the second transaction

This over-estimation carries over and may lead to unexpected GOAWAYs.

@lucasdicioccio

This comment has been minimized.

Contributor

lucasdicioccio commented May 25, 2017

For the record, I've been hammering a warp-tls-enabled toy server with my temptative HTTP2 client at https://github.com/lucasdicioccio/http2-client .

If you want a repro case, use the exe in the above link and use this for server:

server :: IO ()
server =
    runTLS defaultTlsSettings (Warp.setLogger (\r s n -> print r) $ Warp.defaultSettings) app
  where
    app req f = f $ responseBuilder status200 [ ("Content-Type", "text/plain")
                                              , ("Content-Length", "5")
                                              ] $ copyByteString "HELLO"
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented May 26, 2017

LGTM. But I have a comment.

@@ -232,9 +232,12 @@ control FrameGoAway _ _ Context{controlQ} = do
control FrameWindowUpdate header bs Context{connectionWindow} = do
WindowUpdateFrame n <- guardIt $ decodeWindowUpdateFrame header bs
!w <- (n +) <$> atomically (readTVar connectionWindow)
!w <- atomically $ do

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto May 26, 2017

Contributor

Why don't you use modifyTVar'?

This comment has been minimized.

@lucasdicioccio

lucasdicioccio May 26, 2017

Contributor

Last time I checked (and I was sad) TVar.modifyTVar returns unit, unlike modifyIORef.

As per the strictness, the strict pattern binding '!w' will evaluate the thunk.

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto May 26, 2017

Contributor

Last time I checked (and I was sad) TVar.modifyTVar returns unit, unlike modifyIORef.

You are right. This is my fault. Never mind.

As per the strictness, the strict pattern binding '!w' will evaluate the thunk.

Yes. Please do so.

This comment has been minimized.

@lucasdicioccio

lucasdicioccio May 26, 2017

Contributor

No worries :-).
For the point about strictness, I think the bang pattern at the output of the atomically call in my current patch is enough. It's true I could seq the thunk from within the STM. I believe it does not matter much performance-wise here: either concurrent threads will wait more in the STM or evaluating the thunk themselves. But if I'm wrong I'll be happy to learn something :-) (maybe I overlooked some bit about the senders).

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto May 29, 2017

Contributor

I worry about space leak. If you don't have a strong reason to use lazy evaluation, I would recommend the bang pattern.

This comment has been minimized.

@lucasdicioccio

lucasdicioccio May 29, 2017

Contributor

Ack. I've added a strictness annotation from within the STM (cf. a48c7aa.) In that particular instance since w1 was returned and immediately evaluated by the bang pattern I guess it was fine.

I can rebase on master and squash if you prefer a clean history. Let me know.
Thanks for the review!

This comment has been minimized.

@kazu-yamamoto

kazu-yamamoto May 30, 2017

Contributor

No need for squash but rebase is great.

This comment has been minimized.

@lucasdicioccio

lucasdicioccio May 30, 2017

Contributor

Thanks. I've rebased (and made the commit message more precise for the 2nd commit).

lucasdicioccio added some commits May 25, 2017

Guarantee atomicity of WINDOW_UPDATE increments.
The current code may lead to over-estimating the window size.
The over-estimation occurs as follows:
- the initial window state is W
- the receiver observes a window update and computes W+K in a first
  transaction
- the sender decrease the window by the payload size P, hence the window
  state is set to W-P
- the receiver writes W+K in the second transaction

This over-estimation carries over and may lead to unexpected GOAWAYs.

@kazu-yamamoto kazu-yamamoto self-requested a review May 30, 2017

@kazu-yamamoto

Very good.

@kazu-yamamoto kazu-yamamoto merged commit b4f26d1 into yesodweb:master May 30, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented May 30, 2017

Merged. Thank you for your contribution!

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