Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Early websocket closure resulting in websocket connections not being instantiated. #377

Closed
arsanjea opened this issue Mar 19, 2018 · 29 comments

Comments

@arsanjea
Copy link

With [email protected], on the client side, the following error prevents the client from logging in"
WebSocket connection to 'wss://url/subscriptions' failed: WebSocket is closed before the connection is established.

The websocket connection is closed by checkMaxConnectTimeout before the timeout value defined in the client options is reached; which results in websocket subscriptions not being instantiated.
I see the timeout is calculated by this.maxConnectTimeGenerator.duration(); however, this duration is well below the timeout limit passed by the client.

@mxstbr
Copy link
Contributor

mxstbr commented Mar 19, 2018

Is that what's fixed by #368?

@arsanjea
Copy link
Author

I am actually not sure. After troubleshooting the issue, it seems like subscriptions-transport-ws will close the connection after only 1200 ms if it hasn't received a response from the server (it uses Backoff, with a hard coded start of 1000ms and a hard coded factor of 1.2).
It subsequently keeps trying to connect every few 1000's of ms and closes the connection before the handshake can happen.
I might not be setting it up properly, my options are:

timeout: 60000,
reconnect: true,
connectionParams:....
}```

@arsanjea
Copy link
Author

arsanjea commented Mar 20, 2018

As a temporary fix to this issue, I run the following after the client's instantiation:
subscriptionClient.maxConnectTimeGenerator.duration = () => subscriptionClient.maxConnectTimeGenerator.max
This results in only one subscription closure and my app actually working.

This is the line where things go awry:

this.close(false, true);

@gijo-varghese
Copy link

I'm also having the same problem in 0.9.7. Is it an issue with the server or client?

@gijo-varghese
Copy link

@arsanjea Thanks a lot. You saved our lives.
link.subscriptionClient.maxConnectTimeGenerator.duration = () => link.subscriptionClient.maxConnectTimeGenerator.max

That one fixed my issue.

baconz pushed a commit to PhiloInc/subscriptions-transport-ws that referenced this issue Apr 24, 2018
@mattkrick
Copy link
Contributor

in the big picture, why does maxConnectTimeGenerator exist?
Seems like if we got rid of it & implemented a proper onerror websocket client callback everything would be solved.

@Amareis
Copy link

Amareis commented May 16, 2018

Oh, it's really stupid issue. Will it be resolved in any near release?

@gijo-varghese
Copy link

Is this the same library used in React Graphql?

@slawomir-brzezinski-at-clarksons
Copy link

slawomir-brzezinski-at-clarksons commented Dec 17, 2018

in the big picture, why does maxConnectTimeGenerator exist?
Seems like if we got rid of it & implemented a proper onerror websocket client callback everything would be solved.

I second that. As a user, I will be interested in two timeouts separately: the connects (especially the initial one, which I might treat specially, because it often means I'll have to tell user that the app is unavailable), and the keep-alive. Currently, not only they're clumped together under one timeout option, but also the connect is actually very frustratingly starting with 1000ms and ramping up to the user setting rather slowly. Interestingly, if the delay is caused by network congestion, it will congest the network even more and the user will get his connection later.

Also notably, TCP protocol does have a built-in retransmission which works as early as during the handshake, so this userspace-code ramping up seems to be reinventing the wheel a bit (I appreciate some apps and network setups might not like the OS defaults, but I don't think a general purpose library like this should take an opinionated approach that all users want this).

Please remove this ramping up, or at least make the default to not do it.
Or at least pull this pr so users can do it themselves without brittle mangling of private members like the current workaround.

@thezjy
Copy link

thezjy commented Feb 20, 2019

@arsanjea But the maxConnectTimeGenerator is private, how do you access it?

@thezjy
Copy link

thezjy commented Feb 20, 2019

Oh, guess you may not be using TypeScript? But even if I apply the workaround by casting to any the error still occurs. :(

@kyunghoon
Copy link

i fixed this by passing lazy: true to the constructor of SubscriptionClient
then calling subscriptionClient.maxConnectTimeGenerator.setMin(3000);

@javierfernandes
Copy link

Same thing happening for me. Establishing the connection to my server seems to take a little bit longer than 1 second. But as this "min" value is hardcoded it makes it worse because it tries and closes it many times.
I was able to workaround it as others with client.maxConnectTimeGenerator.setMin(3000); but It would be good to have this value configurable from options instead of a hardcoded value.

@analytically
Copy link

maxConnectTimeGenerator is private, so I'm unable to call client.maxConnectTimeGenerator.setMin(3000), how did you work around that?

@n1ru4l
Copy link

n1ru4l commented Oct 4, 2019

@analytically // @ts-ignore 😅

@CyrilSiman
Copy link

On my side this problem occur only on Chrome, when react extension is enabled. If I disable react extension / restart chrome this problem disappear.

@Dizzienoo
Copy link

@CyrilSiman That has worked for me also. I wonder if it is due to a recent update to the React Extension as this problem only arose in the past week or so for me

@FedeBev
Copy link

FedeBev commented Oct 7, 2019

That @arsanjea workaround worked for me too, what about the roadmap to fix this issue?

@UglyHobbitFeet
Copy link

UglyHobbitFeet commented Oct 9, 2019

I'm coming from vertx-web/graphql subscriptions and having the same problem. For a temp fix I modified: node_modules/subscriptions-transport-ws/dist/client.js by changing

  SubscriptionClient.prototype.createMaxConnectTimeGenerator = function () {
    var minValue = 1000;

to

SubscriptionClient.prototype.createMaxConnectTimeGenerator = function () {
    console.log('Note - This is a temp workaround. You modified the official release');
    var minValue = 10000;

@adamtysonsmith
Copy link

adamtysonsmith commented Oct 18, 2019

Disabling react dev tools (and relaunching chrome) also fixed this issue for me

@matthewoates
Copy link

Any idea why react-dev tools is causing this issue? I was able to verify that turning the extension off fixed the issue for me as well.

I have a hard time believing that the extension is blocking the browser for 2,000+ms. Sometimes the client has to reconnect 4-5 times for the connection to finally work. (1s * 1.2 ^ 5, exponential backoff)

@gaspard
Copy link

gaspard commented Oct 25, 2019

OK, I have some information on what is driving this issue and it has something to do with the way async operations are run in JS. The problem is this:

  • connect is a synchronous function that starts a maxConnectTimeout
  • a LOT of time can happen before there is a yield that allows the actual connection to be started, especially during application bootstrap
  • this explains the relation with dev-tools and such (they increase the time before the connection is even started

A correct solution would be to start the maxConnectTimeout thing in the synchronous code that actually starts the connection (even after the connectParams is run):

  private connect() {
    // ASYNC (native) operation: only starts on yield
    this.client = new this.wsImpl(this.url, this.wsProtocols);

    // SYNC operation: starts now
    this.checkMaxConnectTimeout();

    this.client.onopen = async () => {
      // ASYNC CHECK (even more time can pass)
      if (this.status === this.wsImpl.OPEN) {

Should be changed to:

  // ===> ASYNC CONNECT HERE <===
  private async connect() {
    // ASYNC (native) operation: only starts on yield
    this.client = new this.wsImpl(this.url, this.wsProtocols);

    // ASYNC also operation: starts when connect is started (possibly after app bootstrap)
    this.checkMaxConnectTimeout();

    this.client.onopen = async () => {
      // ASYNC CHECK (even more time can pass but should be OK)
      if (this.status === this.wsImpl.OPEN) {

So whatever the solution, this connect timeout (which I don't really know why it exists) should not mesure application boot time but connection delay (the init message)...

@gaspard
Copy link

gaspard commented Oct 25, 2019

The "ASYNC" idea does not work with a 1s timeout.

// Patching like this works fine for me.:
SubscriptionClient.prototype.createMaxConnectTimeGenerator = function() {
  return {
    duration() {
      return WS_TIMEOUT // mine is at 60000
    },
    reset() {
      // noop
    },
  }
}

@dumorango
Copy link

dumorango commented Apr 19, 2020

As a temporary fix to this issue, I run the following after the client's instantiation:
subscriptionClient.maxConnectTimeGenerator.duration = () => subscriptionClient.maxConnectTimeGenerator.max
This results in only one subscription closure and my app actually working.

This is the line where things go awry:

this.close(false, true);

Thanks for your answer @arsanjea, it helped a lot!

One little trick is that it would only work if you have the lazy flag on since otherwise, it will try to connect within the constructor even before putting the workaround in place.

I guess the other solutions posted that rely on prototype would not have this issue, but I end up going for this one that I found encapsulate the workaround a little bit more.


export class CustomSubscriptionClient extends SubscriptionClient {
  constructor(
    url: string,
    options?: ClientOptions,
    webSocketImpl?: any,
    webSocketProtocols?: string | string[]
  ) {
    // It needs to be forced lazy otherwise it will try to connect before the setting up the following workaround
    super(url, { ...options, lazy: true }, webSocketImpl, webSocketProtocols);

    // Workaround suggested for ISSUE 377 on subscriptions-transport-ws package
    // https://github.com/apollographql/subscriptions-transport-ws/issues/377#issuecomment-375567665
    // @ts-ignore
    this.maxConnectTimeGenerator.setMin(this.maxConnectTimeGenerator.max);
    const { lazy = false } = options || {};
    if (!lazy) {
      // @ts-ignore
      this.connect();
    }
  }
}

@emceelovin
Copy link

i am having this exact issue, and for about a week i thought it was my dotnet core proxy that i am writing. i've been beating myself up and scratching my head over it for days and days. thank you @dumorango for a, what i would consider, more proper solution to gain access to the member functions that are attached to the timer. this fixes the connection issue totally to my custom proxy with our SPA, but the playground still doesn't work with the proxy. im ok with that honestly. its a shame that this still hasn't been fixed. very disappointing, but i think i know why. the apollo client with the websocket transport works with graphql-dotnet-server and the apollo servers, but it will not work with certain server implementations apparently, such as my custom proxy and whatever else people are using. which brings me to my question, what are they doing differently than i am? i've been reading and reading over the grapql-dotnet websocket transport middleware, and just cant seem to pinpoint what would cause that to work with apollo client, but not my server. i would be 100000% ok with this not being fixed on the client side, even though it should be, if i could figure out how to make my server work with the current apollo client.

@lbrito55
Copy link

As @arsanjea pointed out this is the solution i will implement as a workaround in my code:

link.subscriptionClient.maxConnectTimeGenerator.duration = () => link.subscriptionClient.maxConnectTimeGenerator.max

Im still trying to get around my head how this solution would work, and im unable to understand it. If someone could explain me would be nice.

Someone else been having this trouble lately?

This is a pretty random problem and its really frustrating. Sometimes it does connect just fine, until this error happens and it keeps trying to reconnect indefinitely.

@skimi
Copy link

skimi commented Nov 5, 2020

I'm coming from vertx-web/graphql subscriptions and having the same problem. For a temp fix I modified: node_modules/subscriptions-transport-ws/dist/client.js by changing

  SubscriptionClient.prototype.createMaxConnectTimeGenerator = function () {
    var minValue = 1000;

to

SubscriptionClient.prototype.createMaxConnectTimeGenerator = function () {
    console.log('Note - This is a temp workaround. You modified the official release');
    var minValue = 10000;

I had the same temp fix but #675 now provides a solution with the minTimeout option. Issue fixed?

@LavHinsu
Copy link

LavHinsu commented Apr 9, 2021

The issue seems to be solved with a minTimeout field on the websocket client. feature wise it's exactly the same, just that it doesn't time out so the console is not filled with a ton of errors.

@jbyomuhangi
Copy link

jbyomuhangi commented Dec 23, 2021

minTimeout?: number: the minimum amount of time the client should wait for a connection to be made (default 1000 ms)

This is what the docs say about the minTimeout property. Setting this to something larger than 1 second (say 10 seconds) worked for me, so you can have something like:

  const subscriptionClient = new SubscriptionClient(websocketUrl, {
    // ... other properties
    minTimeout: 10000,
  });

hope this helps 🙃️.

@glasser glasser closed this as completed Mar 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests