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

Improve nullability #48

Closed
SebastianStehle opened this issue Oct 8, 2023 · 3 comments
Closed

Improve nullability #48

SebastianStehle opened this issue Oct 8, 2023 · 3 comments
Assignees

Comments

@SebastianStehle
Copy link
Collaborator

The y-crdt library does not null that often. If something fails, you actually get an error indicating what the problem is. Unfortunately the yffi binding is returning null for errors and therefore swallows the actual reason. Very often it is mentioned in the documentation what the problem is.

We should not make the mistake. Lets say you open a transaction in your business logic. What do you do, when the result is null? You have to log this case or throw an exception or something like that and you have to research what the error might have been. It would be more user friendly to just throw the exception. Then the code blows up and is usually handled by an exception handler and logged, like for all other exceptions.

Sometimes the nullability is just wrong, e.g.

public Doc? Clone()
    {
        return ReferenceAccessor.Access(new Doc(DocChannel.Clone(Handle)));
    }

or

public Text? Text(string name)
    {
        var nameHandle = MemoryWriter.WriteUtf8String(name);
        var result = ReferenceAccessor.Access(new Text(DocChannel.Text(Handle, nameHandle)));

        MemoryWriter.Release(nameHandle);

        return result;
    }

In both cases the result can only be null in really buggy situations.

Please assign me, if you agree. Happy to provide a PR.

@LSViana
Copy link
Collaborator

LSViana commented Oct 13, 2023

Hi, @SebastianStehle!

I agree with your proposal to fix and improve the documentation here but I think it's part of a bigger problem that I, on purpose, didn't handle in the library so far: exception handling.

You probably noticed wherever something can go wrong in the library, if it happens there's no handling at all. So, I'd suggest renaming this issue to something like "Improve exception handling" and then we can include items like:

  • Handling unexpected null results
  • Handling invalid parameters (like reading beyond the valid indexes of an array)
  • etc.

However, I don't intend to do that in a "local" way as it's suggested here or, for example, by adding a bunch of if statements throughout the code, I'd like to find a global approach to throw exceptions keeping the Rust stack trace and without crashing the process, for example.

So, if you agree, let's try to make this scope bigger and investigate how to handle this.

@LSViana
Copy link
Collaborator

LSViana commented Oct 13, 2023

Also, please keep in mind that the priority of this kind of task is low for now because I'm still improving the features and memory management.

In the best case, for now, if there are no programming errors, the exceptions shouldn't happen very often.

@LSViana
Copy link
Collaborator

LSViana commented Oct 31, 2023

Hi, @SebastianStehle!

Since there are many issues in a row that could be closed after #55 (I think this is the 4th or 5th already), I'll just go ahead and close them for now.

If you notice there's anything else pending to do, please feel free to re-open any of them or just open new issues so we can start discussions with a clean slate.

@LSViana LSViana closed this as completed Oct 31, 2023
@LSViana LSViana reopened this Oct 31, 2023
@LSViana LSViana closed this as completed Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants