Rxjs: Observable.ajax halts when error callback is not given to progressSubscriber

[Boshen]

RxJS version:

5.2.0

Code to reproduce:

in https://github.com/ReactiveX/rxjs/blob/master/spec/observables/dom/ajax-spec.ts

  it('should call onError when error callback is not given to progressSubscriber', function() {
    const onError = sinon.spy();
    const progressSubscriber = Rx.Subscriber.create(
      () => {},
      /* no error callback */
    );

    Rx.Observable.ajax({
      url: '/flibbertyJibbet',
      responseType: 'text',
      method: '',
      progressSubscriber
    })
      .subscribe(() => {}, onError);

    MockXMLHttpRequest.mostRecent.respondWith({
      'status': 404
    });

    expect(onError).to.have.been.called;
  });

Expected behavior:

Test should pass

Actual behavior:

Error: the object {} was thrown, throw an Error :)

Additional information:

Three confusing bits:

1. Should I just pass a noop function to bypass this behaviour

1. Not sure if progress subscriber should throw the ajax error in ajax observable https://github.com/ReactiveX/rxjs/blob/master/src/observable/dom/AjaxObservable.ts#L366

2. Not sure whether Subscriber logic is correct in https://github.com/ReactiveX/rxjs/blob/master/src/Subscriber.ts#L222-L224
   it still throws when _parentSubscriber is not syncErrorThrowable?

[kwonoj]

so current implementation seems expect progressSubcriber to fully implement Observer<T>, and doesn't consider cases of partial observer. But this seems bit off, may need to update logics to check if given progressSubscriber is partial and hand off error to subscriber?

/cc @trxcllnt for visibility.

[trxcllnt]

The SafeSubscriber logic is technically correct, syncErrorThrowable is an internal flag that tracks whether we should run the unsubscribe logic of an Observable if a consumer throws while the source is synchronously emitting values.

I'm not 100% on the right fix. On the one hand, we could fix AjaxObservable to ignore (or catch and forward) the error thrown by calling progressSubscriber.error(). In the case the error method isn't implemented, the error would be the same as it would have sent to the main Subscriber anyway. A more drastic change would be for Subscriber.create to swallow errors if it doesn't have an error handler, but that'd a breaking change.

I'm also tempted to question why progressSubscriber exists at all, as it seems like we could have a progress: true flag that instructs the AjaxObservable to emit progress events, as well as the values it normally emits.

[Brooooooklyn]

I'm also tempted to question why progressSubscriber exists at all, as it seems like we could have a progress: true flag that instructs the AjaxObservable to emit progress events, as well as the values it normally emits.
I agree.

I was trying to get progress event here https://github.com/ReactiveX/rxjs/issues/2424 , but both of two implement have some problem.

Observable.create((subscriber: Subscriber<ProgressEvent>) => {
  const ajax$ = Observable.ajax({
    url: host,
    body: blob,
    method: 'post',
    crossDomain: true,
    headers: { 'Content-Type': 'application/octet-stream' },
    progressSubscriber: subscriber
  })
    .takeUntil(this.pause$)
    .repeatWhen(() => this.resume$)
  const subscription = ajax$.subscribe()
  return () => subscription.unsubscribe()
})
    .retryWhen(() => this.resume$)

In this case I can't get the ajax$ result, because subscriber was stoped before it can emit ajax result.

const host = `${apiHost}/upload/chunk/${meta.fileKey}?chunk=${index + 1}&chunks=${meta.chunks}`
const subscriber = new Subscriber()
return Observable.ajax({
  url: host,
  body: blob,
  method: 'post',
  crossDomain: true,
  headers: { 'Content-Type': 'application/octet-stream' },
  progressSubscriber: subscriber
})
  .takeUntil(this.pause$)
  .repeatWhen(() => this.resume$)

In this case I can't retry or repeat progressSubscriber.

[benlesh]

Ugh... it shouldn't be progressSubscriber, it should be progressObserver. No one should be newing up Subscriber instances.

Either way, It seems like the proper thing to do would be to send any unhandled errors to the main subscription. Thoughts?

[kwonoj]

It seems like the proper thing to do would be to send any unhandled errors to the main subscription.

: I'm also in favor of this approach.

[Boshen]

Any advice on what change is needed to make this work? I'm happy to create a PR.

[awhillas]

whoo, so I'm confused. I just realised that we're talking about rx.observalbe.dom.ajax which uses progressSubscriber not rx.dom.js's ajax which uses progressObserver, right? I'm also only getting complete called and no next events during an ajax download :(

[varghesep]

@awhillas I'm seeing the same when progressSubscriber is provided. I'm seeing only completion and not any readystate change as mentioned here.

[rendall]

I am reporting the same issue. progressSubonly reports progress complete.

const progressSub = Subscriber.create(n => console.log("progressSub", n), e => console.error("progressError", e), () => console.log("progressSub complete"));

AjaxObservable.create({ url: DATA_URI, progressSubscriber: progressSub });

// --> progressSub complete

[rendall]

For folks looking, a workaround is to use createXHR instead of progressSubscriber, like so:

const request = new XMLHttpRequest();
const getRequest = () => request;
const loadProgressObs = Observable.fromEvent(request, 'progress');
loadProgressObs.subscribe((e) => console.log("progress event", e));
AjaxObservable.create({ url: DATA_URI, createXHR: getRequest });

[benlesh]

This has been fixed in #5618