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

fix attachment lose #900 #918

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

procothesky
Copy link

@procothesky procothesky commented Jul 14, 2023

Fix the problem that when broadcasting a byte array data to multiple clients in the same room, the client receives 450, 452 and other wrong binaryEvent number messages and has 45x probe header messages missing attachments

@mrniko
Copy link
Owner

mrniko commented Jul 14, 2023

Thank you for the suggested changes.

Is the problem in empty byte arrays handling? I see your code does the same except empty byte arrays filtering

@procothesky
Copy link
Author

Thank you for the suggested changes.

Is the problem in empty byte arrays handling? I see your code does the same except empty byte arrays filtering

Thanks for asking, I found two problems when I send byte[] message.

  1. When sending a byte[] message to a room, since multiple clients concurrently call the same Packet.initAttachments(jsonSupport.getArrays().size()) method in PacketEncoder.encodePacket(), this will Leading to concurrent modification, the number of attachments is lost or increased when sending messages, causing the client to receive 45x-["event-name",{"placeholder":"true", "num":0}], and no bytes Data attachment. Under the premise that each client does not need new Packet() for the same byte[] message, we can initAttachments() in advance when creating a Packet to avoid concurrent modification and maximize performance.
  2. In line 262 of EncoderHandler: outBuf.writeBytes(buf) - buf is an attachment of Packet, which calls AbstractByteBuf#writeBytes(io.netty.buffer.ByteBuf). This method will move buf.readerIndex to the position of buf.readableBytes on the first call to writeBytes(). After that, other clients who read the attachment of this Packet will not be able to read any valid data, which will cause only the header 04 in the byte[] attachment received by the subsequent client.
    Not sure if I made it clear, if I didn't, please let me know

@procothesky
Copy link
Author

@mrniko Please review it when you are free #900

@@ -22,9 +22,14 @@
import com.corundumstudio.socketio.store.StoreFactory;
import com.corundumstudio.socketio.store.pubsub.DispatchMessage;
import com.corundumstudio.socketio.store.pubsub.PubSubType;
import io.netty.buffer.Unpooled;
import org.springframework.lang.NonNull;
Copy link
Owner

Choose a reason for hiding this comment

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

spring package can't be used

@@ -259,7 +259,7 @@ private void handleWebsocket(final OutPacketMessage msg, ChannelHandlerContext c
for (ByteBuf buf : packet.getAttachments()) {
ByteBuf outBuf = encoder.allocateBuffer(ctx.alloc());
outBuf.writeByte(4);
outBuf.writeBytes(buf);
outBuf.writeBytes(buf, 0, buf.readableBytes());
Copy link
Owner

Choose a reason for hiding this comment

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

this is what outBuf.writeBytes(buf) exactly does

List<byte[]> bytes = Arrays.stream(data)
.filter(o -> o instanceof byte[])
.map(b -> (byte[]) b)
.filter(b -> b.length > 0)
Copy link
Owner

Choose a reason for hiding this comment

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

there are no empty byte arrays


if (!CollectionUtils.isEmpty(bytes)) {
packet.initAttachments(bytes.size());
bytes.stream().peek(b -> packet.addAttachment(Unpooled.wrappedBuffer(b)));
Copy link
Owner

Choose a reason for hiding this comment

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

the same is done in encodePacket() method

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

Successfully merging this pull request may close these issues.

None yet

2 participants