oatpp / oatpp-websocket

oatpp-websocket submodule.
https://oatpp.io/
Apache License 2.0
85 stars 32 forks source link

how to reject a connection in server side ? #41

Open XDinEuro opened 1 year ago

XDinEuro commented 1 year ago

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 ?

madkote commented 1 year ago

+1

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

lganzzzo commented 1 year ago

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:

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

How to drop connection:

socket.getConnection().invalidate();
madkote commented 2 months ago

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

madkote commented 2 months ago

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 commented 2 months ago

@lganzzzo I see where it comes from https://github.com/oatpp/oatpp-websocket/blob/e5b67adfd3105627ef700ac49308565c93c491f9/src/oatpp-websocket/WebSocket.cpp#L291

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

lganzzzo commented 2 months ago

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:

madkote commented 2 months ago

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:

Issues with this approach:

There are workarounds in oatpp controller and WS

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?