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

how to reject a connection in server side ? #41

Open
XDinEuro opened this issue Jan 24, 2023 · 7 comments
Open

how to reject a connection in server side ? #41

XDinEuro opened this issue Jan 24, 2023 · 7 comments

Comments

@XDinEuro
Copy link

XDinEuro commented Jan 24, 2023

Hi, I am using oatpp-websocket example from https://github.com/oatpp/example-websocket/blob/master/.

I have one use case, the server need to decline new connection when there exists two client.

I tried to change the code here from

void WSInstanceListener::onAfterCreate(const oatpp::websocket::WebSocket& socket, const std::shared_ptr<const ParameterMap>& params) {

  SOCKETS ++;
  OATPP_LOGD(TAG, "New Incoming Connection. Connection count=%d", SOCKETS.load());

  /* In this particular case we create one WSListener per each connection */
  /* Which may be redundant in many cases */
  socket.setListener(std::make_shared<WSListener>());
}

to

void WSInstanceListener::onAfterCreate(const oatpp::websocket::WebSocket& socket, const std::shared_ptr<const ParameterMap>& params) {
    SOCKETS++;
    OATPP_LOGD(TAG, "New Incoming Connection. Connection count=%d", SOCKETS.load());
    if (SOCKETS >= 3) {
        SOCKETS = 2;
        OATPP_LOGD(TAG, "More than two socket connected, will not listen to new ones");
        socket.sendClose();
    }
    else {
        socket.setListener(std::make_shared<WSListener>());
    }
}

It works properly at first few attemps, but after few times the server stop responding to any connections even though I disconnect the previous two clients.

Another issue is, after I call socket.sendClose(),
WSInstanceListener::onBeforeDestroy() is not triggered. Thus I need to manually do SOCKETS = 2.

Is there is better way to gracefully cut down the connections, or more straghtforward,

Is there a way to reject coming connections ?

@XDinEuro XDinEuro changed the title how to close a connection in server side ? how to reject a connection in server side ? Jan 26, 2023
@madkote
Copy link

madkote commented Sep 1, 2023

+1

probably you need to accept the connection and close it from listener

@lganzzzo
Copy link
Member

lganzzzo commented Sep 2, 2023

Hello guys,

TLDR:

socket.getConnection().invalidate();

sendClose just sends a close frame to peer. Then peer has to respond with close frame too.
And only after that both sides actually close the connection. - this is the "graceful way".

But the point is that it isn't always possible to follow graceful way due to multiple reasons:

  • client may not follow the protocol
  • connection issue (client changed carrier and connection just hanged)
  • client application hanged
  • any other reason

Your server has to account for all this (using pings/pongs) and drop bad clients.

How to drop connection:

socket.getConnection().invalidate();

@madkote
Copy link

madkote commented Sep 12, 2024

Thanks @lganzzzo ! indeed this is the proper way to close connection on a condition.

@madkote
Copy link

madkote commented Sep 12, 2024

in example code (taken from above) I still would like to send close frame with the code and reason. So that client can handle that exception and maybe try later socket.sendClose(1008...).

Once the line with socket.sendClose(1008...) is executed, the server prints a debug message

[oatpp::web::protocol::websocket::WebSocket::listen()]:Unhandled error occurred. Message='[oatpp::web::protocol::websocket::WebSocket::readFrameHeader()]: Error reading frame header'

Could you please explain why? how could that be fixed? Can that lead to any memory leaks?

Thanks in advance!

void WSInstanceListener::onAfterCreate(const oatpp::websocket::WebSocket& socket, const std::shared_ptr<const ParameterMap>& params) {
    SOCKETS++;
    OATPP_LOGD(TAG, "New Incoming Connection. Connection count=%d", SOCKETS.load());
    if (SOCKETS >= 3) {
        OATPP_LOGD(TAG, "More than two socket connected, will not listen to new ones");
        socket.sendClose(1008, "More than two socket connected, will not listen to new ones");
        socket.getConnection().invalidate();
    }
    else {
        socket.setListener(std::make_shared<WSListener>());
    }
}

void WSInstanceWenet::onBeforeDestroy(const oatpp::websocket::WebSocket& socket) {
  SOCKETS--;
}

@madkote
Copy link

madkote commented Sep 12, 2024

@lganzzzo I see where it comes from

OATPP_LOGd("[oatpp::web::protocol::websocket::WebSocket::listen()]", "Unhandled error occurred. Message='{}'", error.what());

But is it a proper way to stop the loop by raising exception?

@lganzzzo
Copy link
Member

lganzzzo commented Sep 15, 2024

Hello @madkote ,

socket.sendClose(1008, "More than two socket connected, will not listen to new ones");
socket.getConnection().invalidate();

In this scenario, you close the connection immediately after sending the frame. Most likely, the connection is closed before the actual transmission starts.

What you should try is the following:

  • Send a close frame to the client.
  • Schedule the connection for closing (e.g., place it in some sort of pool where it will eventually be closed).

@madkote
Copy link

madkote commented Sep 17, 2024

Hi @lganzzzo thx for reply. I think me and others who were looking for proper socket closure on onAfterCreate would wish a clear API to handle sockets.

Please consider the scenario (which I assume have other also in mind, but with example of counting sockets on a condition) with current state:

  • controller checks parameters and creates response with WS handler
  • WS handler WSInstanceListener receives already opened and accepted socket connection
  • in case, the counter is above >X (e.g. some resource manager), the one has to send close frame and invalidate connection.
  • the step above will trigger onBeforeDestroy and the connection will be closed at some point.

Issues with this approach:

  • it is up to the client, to check if connection is still open (depending on implementation also in widely used libs, it leads to issues)
  • client might receive or not the close frame, because socket is invalidated

There are workarounds in oatpp controller and WS

  • but it does not look clean - I prefer RAII, where WS acquires resources and also cleans up.
  • onAfterCreate should receive open connection, but still not accepted? this way it is still possible to send a close code and let the client know, why the connection has not been accepted (status code, e.g. 1008)

I understand, that I can perform everything in controller, but how a controller can assign a "clean up" to socket (in case controller takes care of resources) ?

Also, if an exception is raised in onAfterCreate or in onBeforeDestroy - what happens with the socket? is it properly closed?

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

No branches or pull requests

3 participants