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

Added <max_connections> to the websocket server #40

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Jul 27, 2020

This supports setting the maximum number of websocket connections.

Signed-off-by: Nate Koenig [email protected]

Signed-off-by: Nate Koenig <[email protected]>
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Jul 27, 2020
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #40 into ign-launch1 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-launch1      #40   +/-   ##
============================================
  Coverage        28.15%   28.15%           
============================================
  Files                3        3           
  Lines              721      721           
============================================
  Hits               203      203           
  Misses             518      518           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83205fa...a30524b. Read the comment docs.

@nkoenig nkoenig requested a review from german-e-mas July 28, 2020 12:20
Copy link
Contributor

@german-e-mas german-e-mas left a comment

Choose a reason for hiding this comment

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

LGTM! Left some minor comments.

So, if we set the max connections to 0, we basically prevent connections, right? That could come handy for the circuit runs I imagine.

ignerr << "Failed to convert port[" << elem->GetText() << "] to integer."
<< std::endl;
}
igndbg << "Using maximum connetion count of "
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: connetion → connection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a30524b

@@ -374,6 +404,9 @@ void WebsocketServer::OnConnect(int _socketId)
//////////////////////////////////////////////////
void WebsocketServer::OnDisconnect(int _socketId)
{
if (this->connections.find(_socketId) == this->connections.end())
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add the comment Skip invalid sockets here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a30524b

Signed-off-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor Author

nkoenig commented Jul 28, 2020

LGTM! Left some minor comments.

So, if we set the max connections to 0, we basically prevent connections, right? That could come handy for the circuit runs I imagine.

Yeah, that's right.

@nkoenig nkoenig merged commit 0256fd7 into ign-launch1 Jul 28, 2020
@nkoenig nkoenig deleted the limit_websocket_connections branch July 28, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants