balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.84k stars 1.95k forks source link

Sails client never connects if client opens when server is down (and then online). #6798

Open glemiere opened 5 years ago

glemiere commented 5 years ago

Node version: 10.9 Sails version (sails):1.2.2 ORM hook version (sails-hook-orm): irrelevant Sockets hook version (sails-hook-sockets): unknown Organics hook version (sails-hook-organics): unknown Grunt hook version (sails-hook-grunt): unused Uploads hook version (sails-hook-uploads): unknown DB adapter & version (e.g. sails-mysql@5.55.5): irrelevant Skipper adapter & version (e.g. skipper-s3@5.55.5): unknown


The title pretty much says it all. If I open my web app when the server is down, and then turn the server on, sails client will never succeed to connect.

Meaning that io.socket.on('connect'... will never trigger.

Any idea why? Sounds like a bug to fix.

sailsbot commented 5 years ago

@glemiere Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

whichking commented 5 years ago

Hey, @glemiere! Are you getting an error in the terminal that you could paste here? It might help me narrow down where things are failing. Thanks!

glemiere commented 5 years ago

Hey @madisonhicks thanks for that fast reply! I do not get any error per se, however:

So I guess it gets stuck into trying to connect and never fails for or something like that.

My code is within the constructor of a (service) class of an Angular app. Here it is, maybe it'll help:

    this.isServerDown           = true;
    this.isServerDownChange     = new Subject<boolean>();

    if (this.utils.isBrowser()) {
      this.io                     = sailsIOClient(socketIOClient);
      this.io.sails.url           = this.config.apiUrl;
      this.io.sails.autoConnect   = true;
      this.io.sails.reconnection  = true;
      this.io.sails.initialConnectionHeaders = {
        nosession: true
      };

      this.io.socket.on('disconnect', () => {
        this.isServerDownChange.next(this.isServerDown = true);
      });

      this.io.socket.on('connect', () => {
        this.isServerDownChange.next(this.isServerDown = false);
      });
    }

    this.window.addEventListener('online', () => {
      this.io.socket.reconnect();
    });

    this.window.addEventListener('offline', () => {
      this.io.socket.disconnect();
      this.isServerDownChange.next(this.isServerDown = true);
    });
whichking commented 5 years ago

@glemiere, is it the case that your front end is in a separate Angular application? I haven't been able to reproduce this error on my side, and we don't have a test situation set up that dissociates the front and back end like that. Is it possible for you to create a repository reproducing the error? Thanks!

glemiere commented 5 years ago

@madisonhicks doing this right now, sorry for late reply I've been busier than expected

glemiere commented 5 years ago

@madisonhicks here you go: https://github.com/glemiere/repro-angular-sails-rt-bug

Let me know if you have any question.

The app component : https://github.com/glemiere/repro-angular-sails-rt-bug/blob/master/src/app/app.component.ts

The service communicating with sails: https://github.com/glemiere/repro-angular-sails-rt-bug/blob/master/src/app/services/api-call.service.ts

The env where you can change the API url: https://github.com/glemiere/repro-angular-sails-rt-bug/tree/master/src/environments

So you open the app with the SERVER DOWN. Then you turn the server ON And it won't turn green with "service online"

Any other case works perfectly.

By default the app connects to localhost:1337.

PS: You need NodeJS v10.9+. I recommend using NVM for easy version switch.

Oh and thanks for the amazing work done with SailsJS. This framework is totally underrated, I love it! However it'd be nice to implement easier relationship query to avoid doing something like: https://gist.github.com/glemiere/426e2ada35a775d22a92819122798c5e

Also documentation needs a lot of improvement

glemiere commented 5 years ago

@madisonhicks let me know when you've checked this out

whichking commented 5 years ago

Hey @glemiere! Thanks for the repo and the information. I'll start looking into this as soon as I can, and I should have something for you by the end of my shift tomorrow.

glemiere commented 5 years ago

Thank you very much @madisonhicks!

whichking commented 5 years ago

Hey, @glemiere. It seems like perhaps you want to enable io.sails.reconnection onthe Sails side of things? I created a repo of an empty Sails app, and a pull request setting io.sails.reconnection = true here. Is this helpful?

glemiere commented 5 years ago

Hi @madisonhicks! (Happy July 4th :D) Unfortunately it's not helping, if you look at the repo I made for you, I already enabled it doing: this.io.sails.reconnection = true; in https://github.com/glemiere/repro-angular-sails-rt-bug/blob/28d8fd5828ba202d53f246d9099c87cc3526d966/src/app/services/api-call.service.ts#L47

As I said, the problem is that if you open your app while the server is down, sails won't make the attempt to reconnect until the server is back online. The current behavior will only attempt to reconnect to the server if you opened the app while the server was on, and then only the server went down, not if the server was down since the beginning.

So I guess there is something wrong in the sails client, where if the initial connection fails, the attempt to reconnect never happens.

whichking commented 5 years ago

Hi, @glemiere! Ah, okay. Yeah, the documented behavior of reconnect() is to attempt to reconnect if the server has been disconnected, not when the server starts out down. Is this primarily an issue in development, or do you have a case in production where this is giving you trouble?

glemiere commented 5 years ago

Hi @madisonhicks! Still in development/staging for now, soon in production.

Here are the things I believe are wrong with the current sails client:

From that, I think we can work something out. I believe the cleanest solution would be to:

Sounds good? I'm not familiar with that code, but if you send me the repo where the sails client exists I can try to PR something.

glemiere commented 5 years ago

@madisonhicks ? @johnabrams7 ?

johnabrams7 commented 5 years ago

@glemiere Hey, sorry we’ve been a bit busy over the past few days, thanks a ton for all the information and repo so far. We just discussed this as a team and are curious if this is because you’re running into an edge case where: the socket server sails app is offline but then back online before the user has a chance to load a different page or reload the native app? Also, if it's ok to discuss, what kind of app are you building and what does the traffic look like?

Thanks, we look forward to your response 👍

glemiere commented 5 years ago

Hi @johnabrams7! Thanks for the reply, no big deal we all have a life beside open-source hah! Thanks to you guys for showing interest into this issue.

It's totally okay to discuss in the limit of what's reasonable. I'm building a software meant to be used internally by the company I'm working at right now, so the traffic will be far from crazy, however if it works as expected for us we may open it to other organizations which will increase the traffic. It's a progressive web app and is using two nodes, one for the front (angular) and one for the back (sails).

However it's a tool meant to help us monitor our sprints better, so having it real-time ready is definitely a must. As you can see in the repo I gave you, there is only a single page so it's not coming from the user navigating too soon.

I don't think I'm running into any edge case here because I do put the server offline on purpose to simulate a case where the user opens the app when the sails server is down. When back online the sails client does not automatically connect unless I'm refreshing the page. Ideally I'd like to be able to connect to the server once online without the need to refresh the page, as it's an installable PWA, the goal is to stay as close as possible to a mobile app experience.

PS: Have you been able to run the Angular app?

glemiere commented 5 years ago

@madisonhicks ? @johnabrams7 ?

johnabrams7 commented 5 years ago

Hey @glemiere! - just sent you a text - the team is curious to chat with you further on this one. You can also reach Rachael on her twitter at: https://twitter.com/fancydoilies 👍

glemiere commented 5 years ago

@johnabrams7 Okay just got your text! Thanks for reaching out!

JaspervRijbroek commented 5 years ago

@glemiere Have you found a solution to this issue?

glemiere commented 5 years ago

HI @JaspervRijbroek! Unfortunately not. There is no option to support this in Sails as of today and I don't think there will be any for a while (or ever). So that sucks... I'm not using Sails at the time, but you can check https://nestjs.com/. NestJS is a backend framework definitely inspired by Angular. It looks pretty cool and supports micro-services (no built in support for SNS/SQS though).