graceful connection termination #19

New Issue

sb10q · 2020-04-24T15:16:55+08:00

sb10q commented

2020-04-24 15:16:55 +08:00

When the remote end terminates the connection, this is what currently happens:

[     0.000000s]  WARN(runtime::moninj): connection terminated: network error: illegal operation

Not a big issue as there are no other effects but something we should clean up.

When the remote end terminates the connection, this is what currently happens: ```text [ 0.000000s] WARN(runtime::moninj): connection terminated: network error: illegal operation ``` Not a big issue as there are no other effects but something we should clean up.

astro commented

2020-04-28 05:26:00 +08:00

That error code is smoltcp's version of EPIPE.

TcpStream::recv() could check for the socket's state and return None in case of connection termination, but that means wrapping the return value with another Result<Option<R>>. Right now, we just pass smoltcp's Err(Illegal) to the caller.

The futures crate provides Stream/Sink traits with many helpers. While very convenient, I did not use them at this time because the developers have announced that they intent to change the underlying interfaces (AsyncRead/AsyncWrite) because they have discovered io_uring.

What API would make your life easier?

That error code is smoltcp's version of `EPIPE`. `TcpStream::recv()` could check for the socket's state and `return None` in case of connection termination, but that means wrapping the return value with another `Result<`**Option<**`R`**>**`>`. Right now, we just pass smoltcp's `Err(Illegal)` to the caller. The `futures` crate provides `Stream`/`Sink` traits with many helpers. While very convenient, I did not use them at this time because the developers have announced that they intent to change the underlying interfaces (`AsyncRead`/`AsyncWrite`) because they have discovered io_uring. What API would make your life easier?

sb10q commented

2020-04-28 16:02:41 +08:00

Poster

If it simply returns "illegal operation" in this case and if there are no ill effects (e.g. corruption of smoltcp state, more serious problems also resulting in a "illegal operation" error), then we can simply not print an error message when that error code is returned.

astro commented

2020-05-01 07:30:26 +08:00

Yes, it's safe.

astro closed this issue

2020-05-01 08:17:05 +08:00

sb10q commented

2020-05-01 08:51:21 +08:00

Poster

48025339b3 is not a proper fix. There are many other places (basically at every other await?) where the peer may close the connection.

I guess we can let handle_connection return Illegal, but then we simply do not print an error in its caller.

https://git.m-labs.hk/M-Labs/artiq-zynq/commit/48025339b359dfbb569920a431889e6626ff8dc0 is not a proper fix. There are many other places (basically at every other ``await?``) where the peer may close the connection. I guess we can let ``handle_connection`` return ``Illegal``, but then we simply do not print an error in its caller.

sb10q reopened this issue

2020-05-01 08:51:24 +08:00

astro commented

2020-05-02 07:05:32 +08:00

I thought of connection termination at any of the other await? to be unexpected. When receiving the request and its parameters an interruption is actually an error condition that should be logged.

I thought of connection termination at any of the other `await?` to be **unexpected.** When receiving the request and its parameters an interruption is actually an error condition that should be logged.

sb10q commented

2020-05-02 11:44:20 +08:00

Poster

I see. I added a comment to explain that.

sb10q closed this issue

2020-05-02 11:44:24 +08:00

Sign in to join this conversation.

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/zynq-rs#19