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

[Suggestion] Client API redesign. #124

Open
iamalexcater opened this issue Jul 22, 2024 · 1 comment
Open

[Suggestion] Client API redesign. #124

iamalexcater opened this issue Jul 22, 2024 · 1 comment
Labels

Comments

@iamalexcater
Copy link

TL;DR - Exposing the underlying buffer smells wrong. (IMO)

The current client API frequently returns Span or Memory, which ultimately are slices into the buffer owned by the ModbusFrameBuffer type. Subsequent operations involving that buffer are directly observable by consuming code; a return value from one of the read methods is always 'corrupted' by a subsequent read/write. Furthermore, the existing return types enable the underlying buffer to be modified by external code, potentially corrupting the buffer whilst it is being used by the API. (were this activity to be performed asynchronously) At a minimum, ReadOnlySpan and ReadOnlyMemory ought to be returned instead of Span and Memory, respectively, but it would be better to avoid exposing the internal buffer altogether. In my opinion, it would be preferable for the read methods to expect a Span or Memory argument instead; the same memory block would still be affected were the same or overlapping block supplied to multiple read methods in succession, but this would be significantly less surprising for the consuming code. I appreciate that this would involve an additional copy operation, but I believe it would be a significantly cleaner design.

@Apollo3zehn Apollo3zehn added the v6 label Jul 22, 2024
@Apollo3zehn
Copy link
Owner

Thanks for bringing this up. I agree that a possible return value should be read-only and that there is a need to pass your own buffer. I will put this on my v6 release list and try to work on it soon together with other API design changes (endianess handling).

Related to #52

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

2 participants