stellar / js-stellar-sdk

Main Stellar client library for the JavaScript language.
https://stellar.github.io/js-stellar-sdk/
Apache License 2.0
628 stars 315 forks source link

Streaming: surface `err` events as errors? #70

Open Mr0grog opened 8 years ago

Mr0grog commented 8 years ago

This one’s a bit interesting. If you stream payments from a non-existent account, like so:

var server = new StellarSdk.Server('https://horizon-testnet.stellar.org');
var paymentStream = server
  .payments()
  .forAccount('GAA2LEFZVJU63SP5B7C77AJXT5PRYDVCWGLFKNVMJTLZU4IXAMXGAATZ')
  .stream({
    onmessage: function(payment) { /* handle payment */ },
    onerror: function(error) {
      console.log('Error:', error);
    }
  });

You'll just get a message like Error: Event { type: 'error' } over and over again. There’s more useful info in the response, though:

retry: 1000
event: open
data: "hello"

event: err
data: sql: no rows in result set

…but to get it, you'd also have to listen for the err event:

var paymentStream = [payments query].stream({
    onmessage: function(payment) { /* handle payment */ },
    onerror: function(error) {}
  });
paymentStream.addEventListener('err', function(event) {
  console.log('Error:', event.data);
});

…which is both a little inconvenient and hard to discover (no docs). It might be nice if the onerror handler was called when err events occur (even though EventSource.onerror is generally intended for network errors). Barring that, this should at least be documented—and perhaps the object passed to CallBuilder.stream() could take a handler for it.

Additionally, the server closes the connection after this error, but that just causes the event stream to attempt to reconnect and you get the same error over and over again (the basic idea of the EventSource API is that any kind disconnect not explicitly requested by the client was a network interruption of some sort and a reconnect should be attempted).

I imagine there could be some rare scenario where Horizon might send an error in the middle of the stream and keep going, but maybe if an err event is immediately followed by a disconnect, the client should hang up? Or Horizon could perhaps send some kind of "end of stream" event to indicate that the client should hang up/not retry.

Mr0grog commented 8 years ago

Guess this is kind of related to https://github.com/stellar/horizon/issues/119, just from the client perspective. Didn’t see that one before I posted here :grimacing:

shredding commented 6 years ago

I have this error as well, does it mean I can ignore it?

gjermundgaraba commented 6 years ago

Could this be worth fixing? I have the same problem :P

tomquisel commented 5 years ago

Should be fixed by stellar/go#884, which will be released this week in Horizon v0.17. Closing. Please re-open if you continue having problems.

cballou commented 5 years ago

I believe this may need to be re-opened on the js-stellar-sdk side (things look good from my stellar/go patch).

As far as I'm aware in the specification of JS and SSE, the error event should be of type Event and not MessageEvent. I still see no appropriate message bubbling on the JS side, so I would surmise this may be the underlying culprit.

You can see the documented differences here:

Specifically, if you take a look at call_builder.ts, you'll see how the onerror callback event gets triggered via error as MessageEvent:

export interface EventSourceOptions<T> {
  onmessage?: (value: T) => void;
  onerror?: (event: MessageEvent) => void;
  reconnectTimeout?: number;
}

// ... and further down ...

    const createEventSource = () => {
      try {
        es = new EventSource(this.url.toString());
      } catch (err) {
        if (options.onerror) {
          options.onerror(err);
        }
      }

      createTimeout();

      if (es) {
        es.onmessage = (message) => {
          const result = message.data
            ? this._parseRecord(JSON.parse(message.data))
            : message;
          if (result.paging_token) {
            this.url.setQuery("cursor", result.paging_token);
          }
          clearTimeout(timeout);
          createTimeout();
          if (typeof options.onmessage !== "undefined") {
            options.onmessage(result);
          }
        };

        es.onerror = (error) => {
          if (options.onerror) {
            options.onerror(error as MessageEvent);
          }
        };
      }

      return es;
    };

I'm not wholeheartedly sure which is the appropriate Event interface here to bubble the pertinent information to the user, but it would definitely be helpful to finally get this right and put the nail in the coffin.

tomquisel commented 5 years ago

@abuiles what are you thoughts on the best approach here?

dolcalmi commented 4 years ago

seems to be the limit configuration in Horizon: https://github.com/stellar/go/blob/master/services/horizon/internal/handler.go (search handler.streamHandler.ServeStream)

a temporary "fix"/solution would be to add the limit param (max 200) that will give us "more time" before the connection is closed

curl -H "Accept: text/event-stream" "https://horizon.stellar.org/transactions?order=asc&cursor=now&X-Client-Name=js-stellar-sdk&X-Client-Version=3.3.0&limit=200"