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

Downgrade engine_socket transport error "polling request overlap" to 4xx from 5xx #650

Closed
pubudu08 opened this issue Jul 14, 2022 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@pubudu08
Copy link

pubudu08 commented Jul 14, 2022

Describe the bug

Hi Peeps,

I am facing an issue where the sockets are in polling mode, the engine io long-polling algorithm will error out when you have two open GET requests. As of now, the error is treated as 500 error.

This could happen when a client has a faulty configuration in place. So, I am proposing we should treat this error as 400/406/409 code where it is used to tell the client that a fault has taken place on THEIR side.

onDataRequest(req, res) {
if (this.dataReq) {
// assert: this.dataRes, '.dataReq and .dataRes should be (un)set together'
this.onError("data request overlap from client");
res.writeHead(500);
res.end();
return;
}

To Reproduce

The issue is similar to #166 but restricted to polling only. No upgrade to WebSocket is allowed in my case.

You can reproduce the issue in the following ways

  • Using a reverse proxy to proxy WebSocket reproduced
  • Use a browser(firefox) to block WebSocket transport reproduced
  • Using a socket client to proxy WebSocket: not-reproduced
    • transports: [ "polling"]

Once the browser shows the xhr requests(GET) as pending state, copy the request as curl. Execute the curl request via the terminal to reproduce the issue.

Engine.IO client version: 6.1.0

Expected behavior

it indicates a faulty implementation of the engine io long-polling algorithm when you have two open GET requests. The proposed Idea is that we could downgrade data request overlap from client error code listed below as 4xx error instead of 500

res.writeHead(500);

potential error codes - 400/406/409

Thanks!

@pubudu08 pubudu08 added the bug Something isn't working label Jul 14, 2022
@carera
Copy link

carera commented Jul 14, 2022

One way i'd propose to solve this is to make the behaviour of this configurable/overridable so that we don't introduce a breaking change.

@darrachequesne
Copy link
Member

That sounds reasonable, let's schedule this for the next major version. 👍

@darrachequesne darrachequesne added enhancement New feature or request and removed bug Something isn't working labels Nov 20, 2022
@darrachequesne darrachequesne added this to the 7.0.0 milestone Nov 20, 2022
darrachequesne added a commit that referenced this issue May 1, 2023
In both cases, the error comes from the client as it should not send
multiple concurrent requests, so a HTTP 4xx code is mandated.

Related: #650
@darrachequesne
Copy link
Member

Done in 911d0e3, included in version 6.4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants