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

Add http connection limits config #17909

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

stuartwdouglas
Copy link
Member

No description provided.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Looks good, I made a comment about the response (should it be closed / ended with 429?)

if (current == maxConnections) {
//just close the connection
LOGGER.debug("Rejecting connection as there are too many active connections");
event.close();
Copy link
Member

Choose a reason for hiding this comment

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

should we just close or return a 429 response?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically there may not actually even be a request yet (as you can open the connection before sending the request), so its hard to say. The problem with connection limits is that there is not really any graceful way to handle them.

Copy link
Member

Choose a reason for hiding this comment

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

The question is how would the load-balancer in front react to "close". This may even be implementation specific.

@vietj, when you worked on the HTTP proxy for Vert.x, did you look into that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other thing is that connection limits are probably generally less important that concurrent request limits. If the connections are idle they have very low overhead.

@cescoffier
Copy link
Member

@vietj is this something that can be done in Vert.x?

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 15, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 67633e4

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 16 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 extensions/vertx-http/deployment

io.quarkus.vertx.http.ConnectionLimitsTest.testConnectionLimits line 77 - More details - Source on GitHub

@stuartwdouglas stuartwdouglas force-pushed the http-connection-limits branch from 67633e4 to bad5ff6 Compare June 15, 2021 23:54
@cescoffier
Copy link
Member

@stuartwdouglas should merge it as it is? We can improve later.

@stuartwdouglas
Copy link
Member Author

I think it is fine as is, I don't really like the approach but it should work.

@cescoffier cescoffier merged commit 94dd1df into quarkusio:main Jun 16, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jun 16, 2021
@gsmet gsmet modified the milestones: 2.1 - main, 2.0.0.Final Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants