You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "krishvishal (via GitHub)" <gi...@apache.org> on 2025/10/20 05:26:06 UTC
[PR] Fix ws bugs [iggy]
krishvishal opened a new pull request, #2279:
URL: https://github.com/apache/iggy/pull/2279
Bugs fixed:
- TLS handshake failed: This is due to system scenario test expecting plan WS server and in the config TLS for WS was enabled.
- Connection closed log noise: This was happening because the websocket client in benchmark sends `Close` frame to server and drops the connection without waiting for ack from server. Due to this server throws `Io error BrokenPipe`. This is expected in real world usage and we handle this on the server side and downgrade the specific errors to warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(io_uring): fix ws bugs [iggy]
Posted by "krishvishal (via GitHub)" <gi...@apache.org>.
krishvishal commented on code in PR #2279:
URL: https://github.com/apache/iggy/pull/2279#discussion_r2444691300
##########
core/server/src/websocket/connection_handler.rs:
##########
@@ -89,17 +88,39 @@ pub(crate) async fn handle_connection(
);
}
Err(error) => {
- error!("Command was not handled successfully, session: {session}, error: {error}.");
- if let IggyError::ClientNotFound(_) = error {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
- error!("Session: {session} will be deleted.");
- return Err(ConnectionError::from(IggyError::ClientNotFound(
- session.client_id,
- )));
- } else {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
+ match error {
+ IggyError::TcpError | IggyError::ConnectionClosed | IggyError::Disconnected => {
+ warn!(
+ "Client {} closed connection during request processing",
+ session.client_id
+ );
+ return Err(ConnectionError::from(IggyError::ConnectionClosed));
+ }
+ IggyError::ClientNotFound(_) => {
+ error!("Command failed for session: {session}, error: {error}.");
+ sender.send_error_response(error.clone()).await?;
+ return Err(ConnectionError::from(error));
+ }
+ _ => {
+ error!("Command failed for session: {session}, error: {error}.");
+ // try to send error response, but if it fails due to broken pipe, that's ok?
Review Comment:
The changes to `websocket/connection_handler.rs` are done to handle the case where client drops the connection without waiting for the ack from server that the server is closing the connection. Server will think its stream has been closed abruptly.
tungstenite dev said the server has to [handle these situations itself](https://github.com/snapview/tungstenite-rs/issues/114#:~:text=You%20can%20of%20course%20drop%20it%20right%20after%20sending%20Close%20to%20server%2C%20but%20in%20this%20case%20you%20may%20observe%20some%20errors%20in%20the%20logs%20of%20your%20server%20(%22peer%20connection%20reset%20without%20closing%20handshake%22%2C%20%22broken%20pipe%22%20or%20something%20similar).) and its not a real error. So I have done this to reduce false log noise.
Hubert agreed that we can consider this as normal behavior.
But in the case of `tcp/connection_handler.rs` I don't know why this behavior is not seen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ws bugs [iggy]
Posted by "numinnex (via GitHub)" <gi...@apache.org>.
numinnex commented on code in PR #2279:
URL: https://github.com/apache/iggy/pull/2279#discussion_r2444067515
##########
core/server/src/websocket/connection_handler.rs:
##########
@@ -89,17 +88,39 @@ pub(crate) async fn handle_connection(
);
}
Err(error) => {
- error!("Command was not handled successfully, session: {session}, error: {error}.");
- if let IggyError::ClientNotFound(_) = error {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
- error!("Session: {session} will be deleted.");
- return Err(ConnectionError::from(IggyError::ClientNotFound(
- session.client_id,
- )));
- } else {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
+ match error {
+ IggyError::TcpError | IggyError::ConnectionClosed | IggyError::Disconnected => {
+ warn!(
+ "Client {} closed connection during request processing",
+ session.client_id
+ );
+ return Err(ConnectionError::from(IggyError::ConnectionClosed));
+ }
+ IggyError::ClientNotFound(_) => {
+ error!("Command failed for session: {session}, error: {error}.");
+ sender.send_error_response(error.clone()).await?;
+ return Err(ConnectionError::from(error));
+ }
+ _ => {
+ error!("Command failed for session: {session}, error: {error}.");
+ // try to send error response, but if it fails due to broken pipe, that's ok?
Review Comment:
How does the tcp `connection_handler` handle that case ?
##########
core/server/src/websocket/websocket_listener.rs:
##########
@@ -137,10 +137,15 @@ async fn accept_loop(
}
registry_clone.remove_connection(&client_id);
- if let Err(error) = sender_kind.shutdown().await {
- shard_error!(shard_clone.id, "Failed to shutdown WebSocket stream for client: {}, address: {}. {}", client_id, remote_addr, error);
- } else {
- shard_info!(shard_clone.id, "Successfully closed WebSocket stream for client: {}, address: {}.", client_id, remote_addr);
+ match sender_kind.shutdown().await {
Review Comment:
I think the shutdown impl for `Websocket` is inproperly implemented.
`TcpSender` implements it with the use of `shutdown` method on `TcpListener`, which in turn submits a `Shutdown` op to the `io_uring`. I think we have to implement it in similar fashion, since `Websocket` uses `TCP`.
I see that you use write and read buffers, do we need such buffering for `Websocket` ? our `TcpSender` isn't buffered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(io_uring): fix ws bugs [iggy]
Posted by "krishvishal (via GitHub)" <gi...@apache.org>.
krishvishal commented on code in PR #2279:
URL: https://github.com/apache/iggy/pull/2279#discussion_r2444682673
##########
core/server/src/websocket/websocket_listener.rs:
##########
@@ -137,10 +137,15 @@ async fn accept_loop(
}
registry_clone.remove_connection(&client_id);
- if let Err(error) = sender_kind.shutdown().await {
- shard_error!(shard_clone.id, "Failed to shutdown WebSocket stream for client: {}, address: {}. {}", client_id, remote_addr, error);
- } else {
- shard_info!(shard_clone.id, "Successfully closed WebSocket stream for client: {}, address: {}.", client_id, remote_addr);
+ match sender_kind.shutdown().await {
Review Comment:
1) std::net::TcpSream is [shutdown when its dropped.](https://doc.rust-lang.org/std/net/struct.TcpStream.html#:~:text=The%20connection%20will,method.). I've assumed compio::net::TcpStream also follows same semantics. Is it still needed in the case of websocket? I've not seen any rust WS examples in the wild where `shutdown` is called separately. I might be missing something.
2) IMO read and write buffers are needed in case of websocket because TCP is stream oriented and websocket is message oriented. In case of TCP we can get exactly the amount of bytes we need from the stream but in case of websockets it is not the case. Read buffer is needed because websocket messages maynot align with our read boundaries - one WebSocket message might contain multiple Iggy requests or one Iggy message can span multiple websocket messages.
Its the same with write buffer. Each `send` in websocket is a message. If we do same as TCP then we have to send three websocket messages for every Iggy message i.e code, length, payload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(io_uring): fix ws bugs [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #2279:
URL: https://github.com/apache/iggy/pull/2279#discussion_r2444935028
##########
core/server/src/websocket/connection_handler.rs:
##########
@@ -89,17 +88,39 @@ pub(crate) async fn handle_connection(
);
}
Err(error) => {
- error!("Command was not handled successfully, session: {session}, error: {error}.");
- if let IggyError::ClientNotFound(_) = error {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
- error!("Session: {session} will be deleted.");
- return Err(ConnectionError::from(IggyError::ClientNotFound(
- session.client_id,
- )));
- } else {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
+ match error {
+ IggyError::TcpError | IggyError::ConnectionClosed | IggyError::Disconnected => {
+ warn!(
+ "Client {} closed connection during request processing",
+ session.client_id
+ );
+ return Err(ConnectionError::from(IggyError::ConnectionClosed));
+ }
+ IggyError::ClientNotFound(_) => {
+ error!("Command failed for session: {session}, error: {error}.");
+ sender.send_error_response(error.clone()).await?;
+ return Err(ConnectionError::from(error));
+ }
+ _ => {
+ error!("Command failed for session: {session}, error: {error}.");
+ // try to send error response, but if it fails due to broken pipe, that's ok?
Review Comment:
in TCP its not seen because TCP sends FIN_WAIT during shutdown, so it's most of the times graceful shutdown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(io_uring): fix ws bugs [iggy]
Posted by "krishvishal (via GitHub)" <gi...@apache.org>.
krishvishal commented on code in PR #2279:
URL: https://github.com/apache/iggy/pull/2279#discussion_r2444691300
##########
core/server/src/websocket/connection_handler.rs:
##########
@@ -89,17 +88,39 @@ pub(crate) async fn handle_connection(
);
}
Err(error) => {
- error!("Command was not handled successfully, session: {session}, error: {error}.");
- if let IggyError::ClientNotFound(_) = error {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
- error!("Session: {session} will be deleted.");
- return Err(ConnectionError::from(IggyError::ClientNotFound(
- session.client_id,
- )));
- } else {
- sender.send_error_response(error).await?;
- debug!("WebSocket error response was sent to: {session}.");
+ match error {
+ IggyError::TcpError | IggyError::ConnectionClosed | IggyError::Disconnected => {
+ warn!(
+ "Client {} closed connection during request processing",
+ session.client_id
+ );
+ return Err(ConnectionError::from(IggyError::ConnectionClosed));
+ }
+ IggyError::ClientNotFound(_) => {
+ error!("Command failed for session: {session}, error: {error}.");
+ sender.send_error_response(error.clone()).await?;
+ return Err(ConnectionError::from(error));
+ }
+ _ => {
+ error!("Command failed for session: {session}, error: {error}.");
+ // try to send error response, but if it fails due to broken pipe, that's ok?
Review Comment:
The changes to `websocket/connection_handler.rs` are done to handle the case where client drops the connection without waiting for the ack from server that the server is closing the connection. Server will think its stream has been closed abruptly.
tungstenite dev said the server has to [handle these situations itself](https://github.com/snapview/tungstenite-rs/issues/114#:~:text=You%20can%20of%20course%20drop%20it%20right%20after%20sending%20Close%20to%20server%2C%20but%20in%20this%20case%20you%20may%20observe%20some%20errors%20in%20the%20logs%20of%20your%20server%20(%22peer%20connection%20reset%20without%20closing%20handshake%22%2C%20%22broken%20pipe%22%20or%20something%20similar).) and its not a real error. So I have done this to reduce false log noise.
Hubert has agreed that we can consider this as normal behavior when I discussed the same with him before proceeding.
But in the case of `tcp/connection_handler.rs` I don't know why this behavior is not seen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(io_uring): fix ws bugs [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #2279:
URL: https://github.com/apache/iggy/pull/2279#discussion_r2444937292
##########
core/server/src/websocket/websocket_listener.rs:
##########
@@ -137,10 +137,15 @@ async fn accept_loop(
}
registry_clone.remove_connection(&client_id);
- if let Err(error) = sender_kind.shutdown().await {
- shard_error!(shard_clone.id, "Failed to shutdown WebSocket stream for client: {}, address: {}. {}", client_id, remote_addr, error);
- } else {
- shard_info!(shard_clone.id, "Successfully closed WebSocket stream for client: {}, address: {}.", client_id, remote_addr);
+ match sender_kind.shutdown().await {
Review Comment:
@krishvishal agreed, we want to have read buffer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(io_uring): fix ws bugs [iggy]
Posted by "numinnex (via GitHub)" <gi...@apache.org>.
numinnex merged PR #2279:
URL: https://github.com/apache/iggy/pull/2279
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org