Skip to content
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

Allow setting number of worker threads and other HTTP/2 setting params #178

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

FinleyMcIlwaine
Copy link
Collaborator

@FinleyMcIlwaine FinleyMcIlwaine commented Jul 3, 2024

We now support setting the SETTINGS_INITIAL_WINDOW_SIZE and SETTINGS_MAX_CONCURRENT_STREAMS settings via http2. We also support setting the initial connection window size, and the number of server worker threads.

The default values we set for these help us avoid the window size exhaustion deadlock that resulted in
#168.

Resolves #145

@FinleyMcIlwaine FinleyMcIlwaine requested a review from edsko July 3, 2024 05:56
We now support setting the `SETTINGS_INITIAL_WINDOW_SIZE` and
`SETTINGS_MAX_CONCURRENT_STREAMS` settings via `http2`. We also support setting
the initial connection window size, and the number of server worker threads.

The default values we set for these help us avoid the window size exhaustion
deadlock that resulted in
[#168](#168).
@@ -41,7 +41,7 @@ data GrpcError =
-- | Invalid argument
--
-- The client specified an invalid argument. Note that this differs from
-- 'GrpcFailedPrecondition': 'GrpcInvalidArgumen'` indicates arguments that
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, wow, thanks for catching these!

@edsko
Copy link
Collaborator

edsko commented Jul 4, 2024

Thanks for the update. I've pushed two more commits, one that avoids hardcoding the default of 8 into grapesy by doing

          HTTP2.numberOfWorkers =
              fromMaybe
                (HTTP2.numberOfWorkers HTTP2.defaultServerConfig)
                (fromIntegral <$> serverOverrideNumberOfWorkers)

instead of having hte figure 8 there (and similarly in other places), and one commit to fix some code layout (long lines).

@edsko
Copy link
Collaborator

edsko commented Jul 4, 2024

Although with kazu-yamamoto/http2#130 that "number of workers" might not be relevant at all anymore? I'll verify.

@edsko edsko merged commit 1d38f88 into main Jul 4, 2024
12 checks passed
@edsko edsko deleted the finley/145 branch July 4, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make HTTP2 window size configurable
2 participants