Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print methods of the Console effect should not depend on Abort[IOException] #1037

Open
rcardin opened this issue Jan 20, 2025 · 21 comments
Open
Labels

Comments

@rcardin
Copy link

rcardin commented Jan 20, 2025

The Scala Console.println doesn't throw an IOException. The method uses the Java PrintStream type, which fails silently in case of errors. So, the kyo.Console effect should not depend upon an Abort[IOException] for printing text on the console.

We should rewrite the code such that the examples in the documentation will change accordingly:

val b: Unit < IO = Console.print("ok")

// Print to stdout with a new line
val c: Unit < IO = Console.printLine("ok")

// Print to stderr
val d: Unit < IO = Console.printErr("fail")

// Print to stderr with a new line
val e: Unit < IO = Console.printLineErr("fail")

// Explicitly specifying the 'Console' implementation
val f: Unit < IO = Console.let(Console.live)(e)

See PrintWriter and PrintStream never throw IOExceptions for further details.

@sideeffffect
Copy link
Contributor

Or rather, in the relevant Console operations, run the PrintWriter#checkError /PrintStream#checkError method and raise a Kyo error if needed.

@rcardin
Copy link
Author

rcardin commented Jan 21, 2025

I think here we need a decision by @fwbrasil ☺️.

By the way, since the exception suppression was intentional, I would give hope to Java designers. Do you want your whole application to stop working because you can't write logs? There are cases when I would say yes and cases when I would say no 🤷‍♂️.

@rcardin
Copy link
Author

rcardin commented Jan 21, 2025

The problem is that we've not thrown IOException. For example, it follows the code inside the PrintStream:

public void write(int b) {
    try {
        if (lock != null) {
            lock.lock();
            try {
                implWrite(b);
            } finally {
                lock.unlock();
            }
        } else {
            synchronized (this) {
                implWrite(b);
            }
        }
    }
    catch (InterruptedIOException x) {
        Thread.currentThread().interrupt();
    }
    catch (IOException x) {
        trouble = true;
    }
}

We would need a dedicated domain error.

@sideeffffect
Copy link
Contributor

Do you want your whole application to stop working because you can't write logs?

That should be up to the application developer. I believe it's trivial to suppress/ignore errors in Kyo. But for that to happen, the errors must not be hidden from the application developer in the first place.

@rcardin
Copy link
Author

rcardin commented Jan 21, 2025

You're right. The only concern is that we cannot access the original IOException.

@johnhungerford
Copy link
Contributor

This is interesting. Looks like ZIO makes the same mistake. I'm kind of inclined to drop the IOException rather than make some effort to uncover the failure because whether or not people are aware of it, this is the behavior they are used to and will be expecting.

@fwbrasil
Copy link
Collaborator

I agree with dropping the pending Abort[IOException] since it matches the behavior of the underlying implementation and simplifies effect tracking. It'd be nice to also expose a method to check for errors, though

@rcardin
Copy link
Author

rcardin commented Jan 23, 2025

I can work on this 😄

@sideeffffect
Copy link
Contributor

this is the behavior they are used to and will be expecting

That may be true. But that doesn't change the fact that errors may happen. And if the Kyo signature doesn't reflect this, then it's lying to its users.

matches the behavior of the underlying implementation

True, but the underlying implementation itself is designed in a ways which is misleading to its users.

simplifies effect tracking

All the work to build this sophisticated effect tracking system, only to not use it when needed? :)

It'd be nice to also expose a method to check for errors

Isn't Abort[...] precisely such Kyo-idiomatic way to check for these errors?

@johnhungerford
Copy link
Contributor

johnhungerford commented Jan 23, 2025 via email

@rcardin
Copy link
Author

rcardin commented Jan 23, 2025

Despite the final solution, we need to fix the current implementation since it's saying it can fail with an IOException, but it can't 🤷‍♂️

@fwbrasil
Copy link
Collaborator

fwbrasil commented Jan 23, 2025

I had in mind exposing a single method like Console.checkErrors: Unit < (IO & Abort[IOException]).

All the work to build this sophisticated effect tracking system, only to not use it when needed? :)

To be frank, for some time I wasn't even convinced that tracking failures in Kyo's APIs made sense. This is a good example since people have been using these Java APIs for many years. I imagine the vast majority don't know of this quirk and it didn't even matter. I can see more value in tracking failures now, especially given Kyo's more flexible support allowing proper handling of specific failures, but I don't see it as a all-or-nothing decision.

In the case of Console, it's a common API for newcomers to use so reducing the effect tracking overhead can easily outweigh the benefits of exposing Abort[IOException]. Also, people might use it for debugging and the introduced pending effect can require additional changes. I believe a checkErrors method and documentation of the behavior via scaladocs is enough to provide a good user experience in general.

@fwbrasil
Copy link
Collaborator

@hearnadam could you share your thoughts on this?

@hearnadam
Copy link
Collaborator

Given the precedence of Java doing the 'wrong' thing, we probably should too. Yes, it would be 'optimal' in some senses to do the right thing (validating the program works as expected/ensuring output), however I think the usability concern of debugging/newcomers is reasonable. I wouldn't expect production projects to use Console, though I could be wrong.

@rcardin
Copy link
Author

rcardin commented Jan 24, 2025

Thanks to everybody for sharing his point of view, so, summing up:

  1. We'll remove the Abort[IOException] effect from the kyo.Console.print family of methods
  2. We'll add a Console.checkErrors: Unit < (IO & Abort[IOException]) to check if there was an error.

Is it correct?

@fwbrasil
Copy link
Collaborator

Yes, I think that's it. About checkErrors, in case we don't have access to the underlying exception, we could return a Boolean < IO.

@rcardin
Copy link
Author

rcardin commented Jan 24, 2025

Ok, let's do it!

@hearnadam
Copy link
Collaborator

@rcardin are you still interested in PR'ing this?

@rcardin
Copy link
Author

rcardin commented Jan 29, 2025

Yep, sorry, I’m having a busy week. Do we have a deadline?

@hearnadam
Copy link
Collaborator

No, take your time!

@rcardin
Copy link
Author

rcardin commented Jan 29, 2025

I will try to open the PR at the latest by this Friday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants