[rstream] Error handling bug/inconsistency?

[jamieowen]

Hi,

This might be less of a bug, but more inconsistency / confusion from an api point of view.
That is now i've learnt the errors of my ways at least - the hard way! )

I thought i'd mention either way...

But when using the reactive() helper and attaching scoped error handlers ( see below ) , any initial errors on startup fail silently and become difficult to track down unless attaching a logger. The api kind of encourages you down a few confusing rabbit holes...

By error handlers i mean :

sub.error = ()=>{
  // scoped handler 
}

sub.subscribe({
  error:()=>{
     // subscription based handler.
  }
})

Here's some cases where errors are handled / or not... ( from code sandbox : https://codesandbox.io/s/blue-cookies-3nk5x?file=/src/index.ts )

/**
 * Subscription based error handler.
 * Works correctly.
 */
reactive(10).subscribe({
  next: () => {
    throw new Error("Subscription based handler works correctly.");
  },
  error: (err) => {
    document.write(`<h3>${err.message}</h3>`);
  }
});

/**
 * Scoped handler to subscription.
 * Is never caught unless a logger is attached.
 */
reactive(10).subscribe({
  next: () => {
    document.write(`<h3>Scoped handler is never called.</h3>`);
    throw new Error("Does not catch. ");
  }
}).error = (err) => {
  console.log("This is never handled");
};

/**
 * But.. will work after first emit and error is attached.
 */
const val = reactive(10);
val.subscribe({
  next: (v) => {
    if (v === 20) {
      throw new Error("Scoped after first emit is ok.");
    }
  }
}).error = (err) => {
  document.write(`<h3>${err.message}</h3>`);
};

val.next(15);
val.next(20); // error ok

/**
 * Transform causes 'illegal state: operation not allowed in state 3',
 * ( the subscription cannot attach ).
 */
try {
  reactive(10)
    .transform(
      map(() => {
        console.log("map");
        throw new Error("Illegal state");
      })
    )
    .subscribe({
      error: () => {
        console.log("Never called");
      }
    });
} catch (err) {
  document.write(`<div>transform causes illegal state.</div>`);
  document.write(`<h3>${err.message}</h3>`);
}

/**
 * Finally transform with scope will not emit unless a logger is attached.
 */
reactive(10).transform(
  map(() => {
    throw new Error("Illegal state");
  })
).error = () => {
  console.log("ERROR");
};

I'm not sure if this could be fixed to support the error = ()=> handler on initial error?
Or may be just better to attach error handlers and then call next()

What do you think?

[postspectacular]

Thanks for the effort in documenting your troubles - and am sorry you're having them in the first place. Will take a closer look ASAP, but probably will take me until the weekend...

All I can tell you right now is that I've never used (or seen) the kind of "scoped error" attachment you're showing here and it seems wrong to me. I.e. a subscriber object being passed to .subscribe() will immediately receive a stream's value the moment it becomes attached and so if you attempt to do .subscribe(...).error = (e) => {...} then your error handler only exist post-factum, but not yet when the error actually occured (in your cases) and hence these error handlers should always be provided as part of the subscriber object (unless you've got other error handling in the next() call tree). I will create some more snippets to illustrate, but as I said, it will take me a few days...

[postspectacular]

One thing you could try (or we could add) is something along these lines:

const withErrorLogging = (next) => ({ next, error(e) { console.log(e); } });

reactive(10).subscribe(withErrorLogging((x) => { throw new Error(`bad: ${x}`); }));
// Error: bad: 10

The cases with .transform() are of a somewhat different nature, but will explain later...

[jamieowen]

Cheers, no rush. Thought it was best to document..

I realise it was more my use of the error handler after the subscribe/post-factum that was the problem. And had been blindly using the 'scoped' handlers for error handling after seeing it here #125.

I will definitely stick to subscriber objects from now on!

Not sure if there is anyway to warn / throw in those cases if no known handler exists..
May be it would be useful to have a section on error handling in the readme..

Grand work either way!! :)

[postspectacular]

I've just committed a change which forces errors to be logged to the console even if a sub doesn't specify an error handler and if the default NULL_LOGGER is used. This should improve DX quite a bit and I'm sorry I didn't think of that earlier...

[jamieowen]

Sounds good!

I have dug into the rstream code a lot more since this last brain lapse...

I think a lot of my confusion came from

• Unsure of what scope was being returned from subscribe() / transform(). And probably expecting 'this' rather than the newly created subscription. ( similar to other chaining style apis )
• Thinking assigning to error() was a custom handler function to add an error handler after a subscription was created.
• Not knowing which subscription the error was executing on.
• Too much stupidity & not enough sleep.

I am now ( hopefully ) more well versed in the nuances of the internals, and have answered all my woes!

I was generally trying to follow the idea of using transform() to manipulate the stream and subscribe() when i wanted to create a 'view' of it. And hadn't really realised the transform() op was returning a new subscription - but thinking it was applying at the current scope.

Either way, It makes sense to have those two methods, it's just a shame you can't add an error handler at the same time.
I have started doing things like :

Either :

str.subscribe({
  next:()=>{},
  error:(err)=>{ throw err}
},comp(
  map((etc)=>etc)
))

or

str.subscribe(subscription({
  next:()=>{},
  error:(err)=>{ throw err}
},{
  xform: map((etc)=>etc)
})),

It's a bit more verbose and i have to add the unnecessary next:()=>, but it gives me error handling - although without the nice transform() naming flow...

Not sure if an error handler coule be passed in as a last arg to transform() ( seems a bit ugly ) or may be a default error should be added to the subscription, that gives you a more appropriate error/console message directing you to the transform().

Anyways, I'm thinking out aloud here...

I'm sure the logger will do it! :)

[postspectacular]

FWIW I'm not too happy/comfortable with the current error handling either, but will think about it some more. The least I should do ASAP is fixing the first arg type for .subscribe() from ISubscriber to Partial<ISubscriber> since actually all of next, done and error handlers are optional already and it should be fine to just pass an empty object... will do that straightaway! In the meantime, here're a few more comments re: your above examples:

I.e. you only need to use comp() if you're using multiple transducers per subscriber and that was one of the reasons for .transform() to exist (it applies comp() for you, else really just syntax sugar)

src.subscribe({}, map((x) => x * 11)) //.single xform

src.subscribe({}, comp(map((x)=> x*11), filter((x) => (x%2)>0))); // multiple xforms

src.transform(map((x)=> x*11), filter((x) => (x%2)>0)); // same as above

src.transform(map(...)).transform(filter(...)) // logically the same, but slower, since it creates two chained subscriptions

Re: your idea about adding an additional/optional error handler arg - we could add it to the optional options arg, but then only for .transform() and the transducer-only version of .subscribe(), else it will be confusing...

src.transform(map((x)=> x*11), filter((x) => (x%2)>0), { error: console.warn });

[postspectacular]

@jamieowen started working on a PR for this, take a look at the expanded commit messages & comments in #279...

[jamieowen]

@postspectacular Looks good. Makes a lot more sense like this. When not mixing the 2 methods.

Do you think it would be good to have an opts only approach on transform too?

src.transform({
  xform: map((etc)=>etc),
  error:()=>{

  }
});

I could then compose reusable transform objects similar to how i might have for subscriptions.

const customTransform = ( mult:number )=> ({
  xform: map(x=>x*mult),
  error:()=>{
  }
})

src1.transform(customTransform(10));
src2.transform(customTransform(42));

For context : I am experimenting with a pipeline style setup for manipulating geometry/buffers on RAF and create 'render subscriptions':

const renderLines = ()=>{
  // setup
  return {
    next:()=>{
       // render
    }
  }
}

str.subscribe(renderLines());
str.subscribe(renderPoints())

I had began using a similar approach for reusable transforms. ( But obviously using the current subscribe() method + xform obj )

[postspectacular]

@jamieowen can this issue be closed now that #281 is in place or still things unclear/unhappy?

[jamieowen]

@postspectacular yep, been meaning to reply to this.

All really great stuff! Looking forward to updating to the new version.

Thanks!

FYI, I did finish those photoshop blend modes as well actually.
http://jamieowen.github.io/glsl-blend/shader-ast.html

Will get back to them soon.. :)