Added err outport for Println.

[MDH0]

This will resolve #859.

Added an err outport to Println and adjusted the go implementation and tests for that.

There is still a problem regarding the For component, because that one doesn't support error handling yet. Should we deal with this yet and if yes, how should we deal with this? Because theoretically (unless I am missing something) we would need an implementation with error handling and an implementation without error handling.

Also another todo is changing the default template and docs for the new Println change.

Another question would be, if we should do the same for Print?

[emil14]

@MDH0

Added an err outport to Println and adjusted the go implementation and tests for that.

Nice job 👍

... problem regarding the For component, because that one doesn't support error handling yet

Good catch. So this one is a bit tricky:

1. Eventually all stream processors must have err outport, including For
2. We gonna have errors.Lift and errors.Must

• errors.Lift convert component that doesn't have err outport to the one that does (error is never sent then)
• errors.Must does the opposite - it converts component with res, err to just res and panics if wrapped component fails

So you have choice here

• implement errors.Must and use it with existing For
• implement errors.Lift and change For, then update for usage where needed

BTW there's an issue related to this (but not about exactly this)

Also another todo is changing the default template and docs for the new Println change.

Default template must be changed because otherwise e2e tests will fail

Another question would be, if we should do the same for Print?

Please check if Go's fmt.Print returns error. I think it does. We follow Go in this regard. If we need to do it, I suggest to make it as a separate task after merging this one

[emil14]

@MDH0

I believe errors package does not exist in main, here's the content for errors/errors.new as I see it. In case you'll need it

(I didn't test it!)

import { runtime }

// Lift converts node that doesn't have error outport to one that does.
// The error outport will forever be blocked.
def Lift<T, Y>(data T) (res Y, err error) {
    handler ILiftHandler<T, Y>
    del Del
    ---
    :data -> [
        handler -> :res,
        false -> switch {
            true -> :err
            _ -> del
        }
    ]
}

pub interface ILiftHandler<T, Y>(T) (res Y, err error)

// Must converts node that have error outport to one that doesn't.
// If handler sends an error, the node will panic.
def Must<T, Y>(data T) (res Y) {
    handler IMustHandler<T, Y>
    panic Panic
    ---
    :data -> handler
    handler:res -> :res
    handler:err -> panic
}

pub interface IMustHandler<T, Y>(T) (res Y, err error)

[MDH0]

Ok, I have more questions regarding this. So first of all, I just checked the Go documentation for their Print's. They throw errors, but they mention, that it is convention, not to worry about errors. There would be the question, how much do we want to follow Go for that? If we really are strict about it, this PR should be closed and we would need to remove the error from Printf.

But independent of this, we have to think about how do deal with For. Because this raises even more questions. The For-handler can throw multiple errors. Therefore, wouldn't we need an error-stream for that?

Alternative ideas I had would be a separate TryFor-component (although I don't really like this idea) or to include an error handler that handles the error seperately (but is against the idea of handling errors in the main component, but might be useful when implementing logging).

[emil14]

@MDH0

They throw errors, but they mention, that it is convention, not to worry about errors

1. Go allows to omit error handling, we don't
2. Go doesn't have ? and require if err != nil boilerplate

Given these statements I would say that both Print and Printf errors should have :err outport:

• Without err there's no way of handling the error, unlike Go where you can omit but if needed can handle
• Single interface for stream-processors: for/map/filter/reduce/etc can just take Println if needed, without any wrappers
• Cognitive overhead is one ? per node, I consider this small

For-handler can throw multiple errors. Therefore, wouldn't we need an error-stream for that?

Not really. As soon as err happens, stream-processor must stop processing next items in a stream, until this stream is closed and new stream is opened. That is, each err message represents a stream, so we don't need stream<error>.

Please note that re-implementing stream processors so they follow this logic is a separate task #861 because it requires internal state

separate TryFor-component

I would rather try to keep APIs minimal and composable instead. E.g. having only one version of For - the one that can handle errors. If our handler doesn't return error (which is weird, because for is for side-effects) we can wrap it For{errors.Lift{MyNode}}

or to include an error handler that handles the error seperately (but is against the idea of handling errors in the main component, but might be useful when implementing logging).

The problem with this solution is that optional dependencies are not supported - if component needs dependency, it always needs it. At least this is the current situation

[MDH0]

Alright, so we are on the same page, I also change the For-component, to return an error if the handler returns an error and in a later step, errors.Lift will be implemented, so components without an error outport will still be able to be used in For.

[emil14]

As soon as err happens, stream-processor must stop processing next items in a stream, until this stream is closed and new stream is opened

By "stop" I mean omit/discard/delete rather than block

[MDH0]

New commit includes the changed For component with error handling.

Open Todos:

• Fix documentation and startup project for new Println implementation
• Add error handling to the Tap component without causing deadlocks

[MDH0]

@emil14 Can you check the documentation on the new commit?

[emil14]

@emil14 Can you check the documentation on the new commit?

@MDH0 sure

optional: I suggest to panic on println:err but we would have to update README.md and docs so I can do that myself later

[emil14]

actually there's no need for lifting here, because println sends err

[emil14]

I've found the reason of deadlock - here PrintAsNum has outports sig, err while For expects res, err. So println sends to :sig but nobody reads from there. It's specific case of the #435

Here's the correct implementation:

import { lists, fmt }

const lst list<bool> = [true, false]

def Main(start any) (stop any) {
	ListToStream<bool>, For<bool>{PrintAsNum}, Wait, Panic
	---
	:start -> $lst -> listToStream -> for
	for:res -> wait -> :stop
	for:err -> panic
}

def PrintAsNum(data bool) (res any, err error) {
	println fmt.Println?
	---
	(:data ? 1 : 0) -> println -> :res
}

Also no need for t.Skip() for this test

[emil14]

@MDH0 unfortunately you have to merge main branch and resolve conflicts in 3 files after I added errors package. I think conflicts should be small, I tried to touch as small stuff as possible

[emil14]

I was able to catch deadlock with neva run -trace e2e/interface_with_imports/main, trace looks like this:

sent | :start | {}
recv | sub_component/printer/println:data | {}
sent | sub_component/printer/println:res | {}
recv | sub_component/printer/__del__:data | {}

It looks very similar to #872 and probably not related to this PR

@MDH0 FYI

[MDH0]

Why are the examples compiling now? This is the opposite of 'it works on my system'.