socket errors not been handled and not fed into Observables

[mike-packetwerk]

What type of issue are you creating?

• Bug
• Enhancement
• Question

What version of this module are you using?

• 2.0.10 (Stable)
• 2.1.0-rc.7 (2.1 Release Candidate 7)
• Other

Write other if any:

Please add a description for your issue:

In flref.ejs you create three new subjects. A single example being:

https://github.com/mean-expert-official/loopback-sdk-builder/blob/master/lib/angular2/shared/models/flref.ejs#L158

Unfortunately you do not handle any socket errors, so it is possible that these new observables will never emit/complete and certainly not error. I'm not meaning in channel errors (ie error information passed back from the server), but rather local socket client library errors.

In my case a timing issues exists such that the initial socket traffic passes over the long polling method before the socket is upgraded to a websocket. In this case a couple of pull/broadcast messages get emitted but before hitting the wire the socket is upgraded. The socket library sends these packets as polling and the server rejects them as it has already upgraded to websocket method and discarded the original id used by polling.

Adding

    this.socket.on('disconnect', error => sbj.error(error));

into each of the affected functions fixes the problem in the sense that my client code can retry the connection/handle the lack of response. However, I'm not clear of the implications with leaking memory/adding an every increasing number of listeners as the socket is really global to the file.

BTW, I tried listening to this.socket.on('error') but it never fired.

[jonathan-casarrubias]

Hi mike thanks for reporting.

I have been adding events to handle these issues, within the fireloop reference will complete the subject on disconnections, but these types of errors events will rather be handled by the realTime service, just like the onReady event, which makes much more sense because connections are handled by the realTime and not from the fireloop reference, remember that there are 3 types of real time functionalities

1.- FireLoop
2.- Standar IO
3.- PubSub

All of them shares socket connection, so within the next release you will have access to the following methods

import { Injectable, Inject } from '@angular/core';
import { IO } from './io.service';
import { JSONSearchParams } from './search.params';
import { LoopBackAuth } from './auth.service';
import { LoopBackConfig } from '../../lb.config';
import { FireLoop } from '../../models/FireLoop';
import { SocketConnections } from '../../sockets/socket.connections';
import { SDKModels } from '../custom/SDKModels';
import { Observable } from 'rxjs/Rx';
import { Subject } from 'rxjs/Subject';

@Injectable()
export class RealTime {

  public IO: IO;
  public FireLoop: FireLoop;
  private connected: boolean = false;
  private socket: any;
  // Note: Creating singleton subject for connection 
  // private connectionSubject: Subject<string> = new Subject<string>();
  private disconnectSubject: Subject<any> = new Subject<any>();
  private unAuthorizedSubject: Subject<any> = new Subject<any>();

  constructor(
    @Inject(SocketConnections) protected connections: SocketConnections,
    @Inject(SDKModels) protected models: SDKModels,
    @Inject(LoopBackAuth) protected auth: LoopBackAuth,
    @Inject(JSONSearchParams) protected searchParams: JSONSearchParams
  ) {}
  /**
  * @method disconnect
  * @description
  * Will disconnect .
  **/
  disconnect(): void {
    this.connected = false;
    this.IO        = null;
    this.FireLoop  = null;
    this.connections.disconnect();
    this.disconnectSubject.next();
  }
  /**
  * @method getConnection
  * @description
  * Return socket connection, enabling only "on" method.
  **/
  getConnection(): { on(event: string, handler: Function): void } {
    return this.connections.getHandler(LoopBackConfig.getPath(), this.auth.getToken());
  }
  /**
  * @method onDisconnect
  * @description
  * Will trigger when Real-Time Service is disconnected from server.
  **/
  onDisconnect(): Observable<null> {
    return this.disconnectSubject.asObservable();
  }
  /**
  * @method onUnAuthorized
  * @description
  * Will trigger when Real-Time Service is not authorized from server.
  **/
  onUnAuthorized(): Observable<null> {
    return this.unAuthorizedSubject.asObservable();
  }
  /**
  * @method onReady
  * @description
  * Will trigger when Real-Time Service is Ready for broadcasting.
  * and will register connection flow events to notify subscribers.
  **/
  public onReady(): Observable<string> {
    let connectionSubject: Subject<string> = new Subject<string>();
    if (this.connected) {
      // Send back to the event loop so it executes after subscription
      setTimeout(() => connectionSubject.next());
    } else {
      this.socket   = this.getConnection();
      this.IO       = new IO(this.socket);
      this.FireLoop = new FireLoop(this.socket, this.models);
      // Fire event for those subscribed 
      this.socket.on('connect', () => {
        console.log('Real-Time connection has been established');
        this.connected = true;
        connectionSubject.next('connected');
      });
      // On server disconnection we need to clean up all and
      // notify the error
      this.socket.on('disconnect', () => {
        console.warn('Disconnected from WebSocket server');
        connectionSubject.error('disconnected');
        this.disconnect();
      });
      // If server is not authenticated we need to fire an event and
      // notify the error
      this.socket.on('unauthorized', (res: any) => {
        console.error('This client is not authorized for real-time connections');
        connectionSubject.error('unauthorized');
        this.unAuthorizedSubject.next();
      });
    }
    return connectionSubject.asObservable();
  }
}

As you can see I have been adding events for onReady, onDisconnect, onUnAuthorized, etc.

so you will be able to listen these from the realTime services and not from the fireloop reference.

Maybe the only missing part is passing back the socket error instead of the disconnected string

[mike-packetwerk]

Ok, but you have I think at least 3 problems remaining:

*) in the disconnect handler, you broadcast the disconnect message before actually processing the disconnect. If I wanted to retry the onReady it creates an infinite loop. ie I think the code should be:

  this.disconnect();
  connectionSubject.error('disconnected');

*) I think you have a kind of race condition with your if (this.connected) statement. Between calling onReady and the connection being established (the connect event) is enough time for many additional onReady calls, all which will create new new FireLoop(this.socket, this.models); The result is the cache of FileLoopRefs is not globally unique.

*) The original problem I reported above still remains. ie calls into FileLoopRef might never return (emit/errror/complete). It seems unnecessarily messy to ask uses of FileLoopRef to have complex Observable pipelines needing to track RealTime.onDisconnect with say FileLoopRef.on simultaneously. Actually, I'm not sure it is possible to get it right. I would be happy to see you directly address this point, and find my logic hole.

[jonathan-casarrubias]

Ok, but you have I think at least 3 problems remaining:

*) in the disconnect handler, you broadcast the disconnect message before actually processing the disconnect. If I wanted to retry the onReady it creates an infinite loop. ie I think the code should be:

  this.disconnect();
  connectionSubject.error('disconnected');

Ok that make sense, I will try to move it before firing the vent.

*) I think you have a kind of race condition with your if (this.connected) statement. Between calling onReady and the connection being established (the connect event) is enough time for many additional onReady calls, all which will create new new FireLoop(this.socket, this.models); The result is the cache of FileLoopRefs is not globally unique.

I think this can be solved by adding a new status "connecting" and wait until it is actually connected o there are no multiple FireLoop and connections, I will play with this and make sure only 1 global fireloop reference is created

*) The original problem I reported above still remains. ie calls into FileLoopRef might never return (emit/errror/complete). It seems unnecessarily messy to ask uses of FileLoopRef to have complex Observable pipelines needing to track RealTime.onDisconnect with say FileLoopRef.on simultaneously. Actually, I'm not sure it is possible to get it right. I would be happy to see you directly address this point, and find my logic hole.

I'm not totally agree with you on this one, because decoupling functionality to later be used with other modules it does not seems messy for me, though I agree what you suggest would make life easier for fireloop only users .

The problem with this project is that it manage several use cases and people asking requests only take care for their specific use case, as I said there are 3 different ways to communicate through websockets using the SDK Builder and all of them share this issue.

Anyway, it is in my future plans to separate the SDKs once the moment arrive

[mike-park]

(Mike from another account)

I understand you have competing requests and 3 subsystems, however this is not a request. I believe this to be a serious bug.

I'd appreciate if you would share example code of how you'd expect an end-user can work successfully with the suggested setup in the face of FireLoopRef.on never returning any event.

[jonathan-casarrubias]

Yes I agree we have a bug in here, I'm working on to resolve it... we are just in disagree where the bug should be fixed, but that my friend is where the big ideas arise, from good brainstorming like this.

Let me finish the approach I'm working on and I will add an example in here.

If you still feel it is not the right approach, lets then discuss and try to get the best from this.

Cheers
Jon

[jonathan-casarrubias]

Ok at the end I was able to handle disconnectios inside the sdk, the fix is transparent for developers.

While you create your fireloop references inside onReady. The SDK will internally handle everything for you.

The onDisconnect, onAuthenticated and onUnAutorized events are not for uou to handle the reconnect but to notify the user through UI that any of these events happen.

Socket io will automatically reconnect when possible and the SDK will recover the client subscriptions state in both server and client.

It will clean all the listeners and onReconnection will automatically setup everything again.

The test application inside this repo is an example. If you see there are no implementation changes, other than subscribing within the app component to each of the connection statuses

btw you also need to upgrade the loopback-component-realtime