Code logic bugs in engine.io-client
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 63.2k
- Forks
- 10.3k
- Avg merge
- 11d 20h
- Merged PRs (30d)
- 2
Description
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
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();
}
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
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);
}
}
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?
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the referenced Socket.java sendPacket/flush and Polling.java doClose/_onData paths, then compare them with the current JavaScript or TypeScript implementation. Reproduce multiple sends before onDrain and closing while polling is opening; done means both lifecycle cases have confirmed behavior and corresponding validation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java, javascript, kotlin, typescript
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 28/100