Code logic bugs in engine.io-client

[Piasy]

Hi, I've been working on porting Java implementation of EngineIO and SocketIO to Kotlin Multiplatform recently.

During the development and test process, I find two code logic bugs. I reported them in socketio/engine.io-client-java #119 and #120, but it seems that this repo hasn't been active for almost 3 years.

I reviewed the latest code of JS implementation, and the bugs are the same.

Bug of Socket's sendPacket and flush implementation

https://github.com/socketio/engine.io-client-java/blob/main/src/main/java/io/socket/engineio/client/Socket.java#L696C1-L712C6

    private void sendPacket(Packet packet, final Runnable fn) {
        if (ReadyState.CLOSING == this.readyState || ReadyState.CLOSED == this.readyState) {
            return;
        }

        this.emit(EVENT_PACKET_CREATE, packet);
        this.writeBuffer.offer(packet);
        if (null != fn) {
            this.once(EVENT_FLUSH, new Listener() {
                @Override
                public void call(Object... args) {
                    fn.run();
                }
            });
        }
        this.flush();
    }

https://github.com/socketio/engine.io-client-java/blob/main/src/main/java/io/socket/engineio/client/Socket.java#L617C1-L627C6

    private void flush() {
        if (this.readyState != ReadyState.CLOSED && this.transport.writable &&
                !this.upgrading && this.writeBuffer.size() != 0) {
            if (logger.isLoggable(Level.FINE)) {
                logger.fine(String.format("flushing %d packets in socket", this.writeBuffer.size()));
            }
            this.prevBufferLen = this.writeBuffer.size();
            this.transport.send(this.writeBuffer.toArray(new Packet[this.writeBuffer.size()]));
            this.emit(EVENT_FLUSH);
        }
    }

When we want to send a packet, we add it to writeBuffer and trigger flush, and we send all packets in writeBuffer to transport, and we remove this amount of packets from writeBuffer in onDrain.

But if we call send multiple times before onDrain is triggered, let's say 3 times, with pkt1, pkt2, pkt3, then we will call transport.send 3 times, with [pkt1], [pkt1, pkt2], and [pkt1, pkt2, pkt3], and that's definitely a wrong behavior.

Bug of Polling transport's close implementation

https://github.com/socketio/engine.io-client-java/blob/main/src/main/java/io/socket/engineio/client/transports/Polling.java#L147C1-L167C6

    protected void doClose() {
        final Polling self = this;

        Emitter.Listener close = new Emitter.Listener() {
            @Override
            public void call(Object... args) {
                logger.fine("writing close packet");
                self.write(new Packet[]{new Packet(Packet.CLOSE)});
            }
        };

        if (this.readyState == ReadyState.OPEN) {
            logger.fine("transport open - closing");
            close.call();
        } else {
            // in case we're trying to close while
            // handshaking is in progress (engine.io-client GH-164)
            logger.fine("transport not open - deferring close");
            this.once(EVENT_OPEN, close);
        }
    }

https://github.com/socketio/engine.io-client-java/blob/main/src/main/java/io/socket/engineio/client/Transport.java#L83C1-L94C6

    public Transport close() {
        EventThread.exec(new Runnable() {
            @Override
            public void run() {
                if (Transport.this.readyState == ReadyState.OPENING || Transport.this.readyState == ReadyState.OPEN) {
                    Transport.this.doClose();
                    Transport.this.onClose();
                }
            }
        });
        return this;
    }

Actually if we call transport.close() while the polling transport is still opening, EVENT_OPEN won't be triggered, because doClose will return immediately, and Transport.this.onClose() will be called, then readyState will be CLOSED, and any further response of poll won't trigger EVENT_OPEN, because code logic below: https://github.com/socketio/engine.io-client-java/blob/main/src/main/java/io/socket/engineio/client/transports/Polling.java#L109C1-L129C11

    private void _onData(Object data) {
        final Polling self = this;
        if (logger.isLoggable(Level.FINE)) {
            logger.fine(String.format("polling got data %s", data));
        }
        Parser.DecodePayloadCallback callback = new Parser.DecodePayloadCallback() {
            @Override
            public boolean call(Packet packet, int index, int total) {
                if (self.readyState == ReadyState.OPENING && Packet.OPEN.equals(packet.type)) {
                    self.onOpen();
                }

                ...
            }
        };

I think the problem is we shouldn't call Transport.this.onClose() in Transport.this.close(), am I right?