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.

Sign up for GitHub

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

Jump to bottom

Duplicate messages emitted after error in onmessage #563

Closed
jcheng5 opened this issue Sep 8, 2021 · 6 comments · May be fixed by #564
Closed

Duplicate messages emitted after error in onmessage #563

jcheng5 opened this issue Sep 8, 2021 · 6 comments · May be fixed by #564
Labels
no-issue-activity

Comments

@jcheng5
Copy link

jcheng5 commented Sep 8, 2021 •
edited

Using xhr-streaming, xhr-polling, or jsonp-polling, if an error is raised in the socket.onmessage event handler, then the next time a message is received SockJS will replay the message that caused the error.

For example, message A is received and the client onmessage event handler throws an uncaught error; later, message B is received, and onmessage is called again with message A.

Repro repo: https://github.com/jcheng5/sockjs-error-test

The problem appears to be here:

sockjs-client/lib/transport/receiver/xhr.js

Lines 40 to 52 in 71876b3

for (var idx = -1; ; this.bufferPosition += idx + 1) {
var buf = text.slice(this.bufferPosition);
idx = buf.indexOf('\n');
if (idx === -1) {
break;
}
var msg = buf.slice(0, idx);
if (msg) {
debug('message', msg);
this.emit('message', msg);
}
}
};

When this.emit('message', msg); throws an exception, this.bufferPosition doesn't get updated (third clause of the for-loop). If you update this.bufferPosition before emitting the message, the problem goes away.
jcheng5 added a commit to jcheng5/sockjs-client that referenced this issue Sep 8, 2021
@jcheng5
Make xhr transports resilient to onmessage errors
767b921 
Fixes sockjs#563
@jcheng5 jcheng5 mentioned this issue Sep 8, 2021
Open
@aronatkins
Copy link

This appears to be the 0.3.4 equivalent code, which updates the buffer position ahead of sending an event and does not suffer from duplicate messages after error:

sockjs-client/lib/trans-receiver-xhr.js

Lines 14 to 24 in 0c70698

that.xo.onchunk = function(status, text) {
if (status !== 200) return;
while (1) {
var buf = text.slice(buf_pos);
var p = buf.indexOf('\n');
if (p === -1) break;
buf_pos += p+1;
var msg = buf.slice(0, p);
that.dispatchEvent(new SimpleEvent('message', {data: msg}));
}
};

@github-actions
Copy link

This issue has been inactive for 30 days. It will be in closed in 5 days without any new activity.

@aronatkins
Copy link

Discussion has been happening in #564; false no-activity.

@github-actions
Copy link

This issue has been inactive for 30 days. It will be in closed in 5 days without any new activity.

@aronatkins
Copy link

False no-activity. Discussion is in #564; waiting for feedback from @brycekahle.

@github-actions github-actions bot closed this as completed Dec 9, 2021
@aronatkins
Copy link

Ping to keep open; #564 is still waiting for feedback from @brycekahle.

whimsicallyson pushed a commit to posit-dev/sockjs-client that referenced this issue May 8, 2024
@jcheng5 @whimsicallyson
Make xhr transports resilient to onmessage errors
9712c24 
Fixes sockjs#563
whimsicallyson pushed a commit to posit-dev/sockjs-client that referenced this issue May 8, 2024
@jcheng5 @whimsicallyson
Make xhr transports resilient to onmessage errors
c5a09c9 
Fixes sockjs#563
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
no-issue-activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make xhr transports resilient to onmessage errors jcheng5/sockjs-client
3 participants
@jcheng5 @aronatkins @brycekahle