EsotericSoftware / kryonet

TCP/UDP client/server library for Java, based on Kryo
BSD 3-Clause "New" or "Revised" License
1.81k stars 415 forks source link

Missing message possible reasons #137

Closed samthaku closed 6 years ago

samthaku commented 6 years ago

Hi,

We are using KryoNet 2.2.1 for our client-Server communication.

We are using pluggable Serialization using Simple Binary Encoding protocol.

We have recently seen issues where Message are disappearing on the wire i.e. Message are sent from the Sender but are not Received in receiver.

Messages sent: 1-4 Client received: 1,3 and 4. 2 went missing

Both Server and Client have read/write buffer sizes of 1MB.

Just wanted to understand if any such issues have been reported before and is there a known fix /reason for these issues?

What possibly can go wrong, as we have checked our code, it looks fine.

crykn commented 6 years ago

Do you use TCP or UDP?

samthaku commented 6 years ago

I'm Using TCP.

samthaku commented 6 years ago

We have found the cause of this issue, The pluggable serialization we are using has some state, and this had to be made thread safe. On Server side each connection has their own write-lock but multiple connections are using the same serialization instance. This Same serialization instance is used to send Keep-Alives which are sent on Server thread. While the application messages are published on Application thread.

mihhon commented 6 years ago

Hi, yes, you use the same instance of Serialization in all the server's Connections. If the implementation of Serialization is not threadsafe, it fails.

Server class

private void acceptOperation (SocketChannel socketChannel) {
    Connection connection = newConnection();
    connection.initialize(**serialization**, writeBufferSize, objectBufferSize);

So, if I can give a suggestion, add please comments on Serialization interface that an implementation should be threadsafe, of even better, add something like SerializationFactory that will be called in accept

Serialization newInstance(String connectionId);

Thanks

RobertZenz commented 6 years ago

@samthaku Please don't forget to close the issue if it is no longer relevant.

mihhon commented 6 years ago

@RobertZenz : it is still relevant. We found the cause of the issue, it is described above. The remedy is to synchronize #write method in Serialization , as it is done in KryoSerialization class. But in this case Serialization is shared among all the clients connections, and all the clients connections block in #write method on the same object.

What is the rationale behind the KryoSerialization implementation - synchronizing on the instance of #read and #write of all the clients connections ?

RobertZenz commented 6 years ago

@mihhon Oh sorry, I parsed it as "we wrote our won serializer which had this problem".

crykn commented 6 years ago

@mihhon The usage of a socket factory is a good idea! I'm currently trying this out in my own fork, do you mind taking a look at it? @samthaku Does it happen that a keep alive-message is sent simultaneously with an application message on the same connection? Or do you only send the keep alive-messages on unused connections?

mihhon commented 6 years ago

@crykn it happens when keep alive-message is sent on "timed out" connections simultaneously with an application message on one of connections

the piece of code below in Server in "Server" thread executes simultaneously with an application thread calling Connection#sendTcp

    long time = System.currentTimeMillis();
    Connection[] connections = this.connections;
    for (int i = 0, n = connections.length; i < n; i++) {
        Connection connection = connections[i];
        if (connection.tcp.isTimedOut(time)) {
            if (DEBUG) debug("kryonet", connection + " timed out.");
            connection.close();
        } else {
            if (connection.tcp.needsKeepAlive(time)) connection.sendTCP(FrameworkMessage.keepAlive);
        }
        if (connection.isIdle()) connection.notifyIdle();
    }
}
crykn commented 6 years ago

Hmm, that means that the serialization object needs to be instanciated once per message and not once per connection, doesn't it?

mihhon commented 6 years ago

once per connection should be enough

On Tue, Aug 15, 2017 at 11:06 PM, damios notifications@github.com wrote:

Hmm, that means that the serialization object needs to be instanciated once per message and not once per connection, doesn't it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/EsotericSoftware/kryonet/issues/137#issuecomment-322495578, or mute the thread https://github.com/notifications/unsubscribe-auth/AdjxP44j_u_un808-Lru5pbk8T23v4SLks5sYbQKgaJpZM4O0SSD .

-- Greetings mihhon

crykn commented 6 years ago

If once per connection is enough, you could try out my fork and report back if it works.

NathanSweet commented 6 years ago

What is the rationale behind the KryoSerialization implementation - synchronizing on the instance of #read and #write of all the clients connections ?

KryoSerialization state is not thread safe: input buffer, output buffer, Kryo instance. Creating a Serialization per connection is not a good solution. An alternate Serialization could be written which uses thread local storage, however this is up to the Serialization implementation and what it needs to be thread safe. I don't think KryoNet needs a change, except possibly documentation.