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

Fix panic on mismatching request/response function codes #254

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

uklotzde
Copy link
Member

Fixes #252.

@uklotzde
Copy link
Member Author

The seconds commit introduces another error variant that replaces std::io::Error in case of mismatching headers between request and response. All error variants include the actual response result.

src/lib.rs Outdated Show resolved Hide resolved
@flosse
Copy link
Member

flosse commented Mar 26, 2024

I'm sorry, where exactly is the fix?

}

// Match function codes of request and response.
let rsp_function_code = match &result {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix (RTU).

Ok(response) => response.function_code(),
Err(ExceptionResponse { function, .. }) => *function,
};
if req_function_code != rsp_function_code {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix (TCP).

@uklotzde
Copy link
Member Author

I am still unsure if it would be more suitable to replace the outer std::io::Error with a new error type or the inner type as done here. Where do protocol errors belong?

@uklotzde uklotzde marked this pull request as draft March 26, 2024 23:08
@uklotzde uklotzde marked this pull request as ready for review March 26, 2024 23:51
@uklotzde
Copy link
Member Author

The alternative approach of replacing the outer std::io::error with a custom error type and preserving Exception as the inner error type turned out to be much more elegant.

The different sources of errors are now separated more consistently, i.e. client/server communication vs. server-side processing.

@flosse
Copy link
Member

flosse commented Mar 27, 2024

I am still unsure if it would be more suitable to replace the outer std::io::Error with a new error type or the inner type as done here. Where do protocol errors belong?

In general I'd look on the layers.

Application -> Modbus -> Transport

So my error design would be

enum MyAppError {
  Modbus(ModbusError),
  // other app specific
}
enum ModbusError {
  Transport(TransportError).
  // other modbus specific
}
enum TransportError {
  // ...
}

So the top level error in this library would be a very specific error where io::Error is just one variant.

@flosse
Copy link
Member

flosse commented Mar 27, 2024

The alternative approach of replacing the outer std::io::error with a custom error type and preserving Exception as the inner error type turned out to be much more elegant.

😂 it looks like you had the same idea 😄

💯

@uklotzde
Copy link
Member Author

Keeping the Exception separate as a not-really-an-error without wrapping it justifies the nested Result pattern.

@flosse flosse merged commit 6aad370 into slowtec:main Mar 27, 2024
10 checks passed
@uklotzde uklotzde deleted the response-error-no-panic branch March 27, 2024 11:50
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

Successfully merging this pull request may close these issues.

tokio-modbus 0.12 entering unreachable code in any response error
2 participants