Update core bindings to Yrs v0.18

LSViana commented 4 months ago

Closes #91.

This PR is a WIP. There are some TODO comments in the code to be checked.

LSViana commented 4 months ago

IMHO we should discuss the is_alive feature. I have discussed this several times with the team and it is actually a solution to prevent unexpected crashes in .NET because of dangling pointers.

What we should do, is to call this method before each access to the underlaying pointer to guard our library and to prevent server crashes that cannot be logged or catched.

I agree. Would it work if we overrode ThrowIfDisposed() in the Branch class to also check for Alive()?

This would allow us to run this check without changing a lot of code since the Branch and all subclasses already use ThrowIfDisposed() before calling all methods (I didn't find any method lacking this check, for example).

LSViana commented 4 months ago

Hi, @SebastianStehle!

Did you have a chance to continue reviewing this PR? I assume you're not finished since there are just a couple of comments and I pushed a lot of changes. If you just didn't have time yet, that's fine too.

I'll probably be away from the computer for the next few days because of a local holiday but I might resume working on the changes during the weekend.

SebastianStehle commented 4 months ago

Yes, I am. I have not reviewed the tests and everything else was looking fine :)

I agree. Would it work if we overrode ThrowIfDisposed() in the Branch class to also check for Alive()?

Would work. But I understand it properly there are 3 methods now:

Get a reference from a pointer. ybranch_id
Check if the reference is valid. ybranch_alive
Get a reference from a pointer. branch_get

I would ...

Create a struct for ybranch_id, so we can distinguish it from actual pointers. Mark this internal.
Always store brand ID, never any pointers.
Provide a method to get the pointer within a transaction and use this in all methods. We could either call the internal alive method in the Disposed check method or just within this method and get rid of the general disposed method.

But in general the branch_id is something internal and we should never store pointers.

LSViana commented 3 months ago

Regarding this:

I would ...

Create a struct for ybranch_id, so we can distinguish it from actual pointers. Mark this internal.

Always store brand ID, never any pointers.

Provide a method to get the pointer within a transaction and use this in all methods. We could either call the internal alive method in the Disposed check method or just within this method and get rid of the general disposed method.

But in general the branch_id is something internal and we should never store pointers.

For item (1), I created the internal struct BranchIdNative so this part is done already. The change here is to remove the public class BranchId and handle it internally only. Is that correct?

For item (2), I'm already storing an instance of the struct BranchIdNative so this part shouldn't change.

For item (3), I'm not sure what you meant. Could you please explain your idea again and maybe the steps you'd use to implement it?

SebastianStehle commented 3 months ago

3) I just meant that we should not store pointers, only IDs. Only use pointers within a method as a temporary variable.

LSViana commented 3 months ago

Okay, this makes sense but I don't see how to reflect this in the code properly.

If you have the time, please feel free to push commits here to show what you mean, it's probably faster and more efficient.

SebastianStehle commented 3 months ago

I have pushed a test commit.

SebastianStehle commented 3 months ago

Perhaps we should focus on the breaking changes first without the ID stuff and then add them in a later reelase.

LSViana commented 3 months ago

@SebastianStehle I'm not sure I understand the latest changes. If we turn the public class BranchId into internal record struct, we can probably just drop it and use the BranchIdNative directly, no?

Besides that, most of the properties of the former public class BranchId aren't used anymore so we can drop them.

Maybe I'm misunderstanding the purpose of the branch IDs.

LSViana commented 3 months ago

Perhaps we should focus on the breaking changes first without the ID stuff and then add them in a later reelase.

I don't have a strong opinion here, I'm fine releasing it all together to move forward faster while the library doesn't have lots of users.

SebastianStehle commented 3 months ago

I just introduced the class to have some shared behavior like dispose handling.

BranchId is like a virtual pointer or reference to solve the problem of dangling pointers and reported crashes. Therefore there is no need to expose that, it is not meant to be used outside.

With all the other incompatibilities I thought it might be easier to focus on them first to update the public API and then solve the internal implementation details. Some methods are not relevant anymore like Branch.ReadTransaction.

LSViana commented 3 months ago

@SebastianStehle Got it!

I understand it better now. I pushed three commits now that I'd like you to review.

If you agree with the changes, we can propagate them to all other shared types that inherit from the Branch class.