false error: NotConnected and possibly some others

[GlenDC]

When a client disconnects on MacOS (I think you are aware of this) we currently get false error logs. It's only logs so is not critical, but it would mean that it shows up in my production logs if I would ever use this project in production.

e.g.

ERROR fast_socks5::server] transfer error: Os { code: 57, kind: NotConnected, message: "Socket is not connected" }

within my code I catch some of those as:

Err(err) => match err.kind() {
            ErrorKind::NotConnected => {
                info!("socket transfer closed by client");
                Ok(())
            }
            ErrorKind::ConnectionReset => {
                info!("socket transfer closed by downstream proxy");
                Ok(())
            }
            _ => {
                traffic_log.err_code = 1;
                Err(SocksError::Other(anyhow!(
                    "socket transfer error: {:#}",
                    err
                )))
            }
        },

I do not think this code is complete or the best, but it is something in hopefully the right direction?
Probably we can make some util code for this as there might be a couple of locations where we want to
treat certain expected errors as non-errors.

[dizda]

You mean the library throw an error level log eventually because of this line: https://github.com/dizda/fast-socks5/blob/master/src/server.rs#L650 ?

We could, indeed decrease the log level for that error using a pattern matching, not sure if this would be the right solution though as some people may expect it to throw an error. (I'm guessing)

The other solution would be to restrict the LOG level of your binary to switch production logs by maybe not showing fast-socks5 library logs output.

By doing this way:

RUST_LOG=warn replacing with RUST_LOG=your_binary_name=warn

Consequently, the logs from fast-socks5 won't be shown anymore, just the logs printed out from your binary will be shown.

[GlenDC]

We could always turn these into a specific kind of error (new variant for the SocksError) such that it can still be seen as an error for those who want (and wou be so by default) but that for others it can be easily caught with a single pattern. On top of that it would allow to keep track of the bytes also for that error variant. But the latter is just a bonus rather than really the reason to do so.

[GlenDC]

Demoting things to warn I am not a fan of personally. Sometimes I might see it a bit too black and white, but I do prefer to either have something be an error, or just no error at all.

That being said. Not sure it is ever the purpose of a library to output errors. Errors are normally to be handled. So if the error is already returned (which is should be, right?) than the logging is a bit unneeded. For brinaries you can have errors logged as a possible error handling, but I feel that a library should only ever have either no logs, or perhaps a minimal amount of info traces and whatever useful debug traces. But that might just be me.

[GlenDC]

Seems these errors are avoided if I switch around the streams in my bi-directional call

[GlenDC]

Never mind. Sometimes I get the read/written bytes count, but most of the times not.
I believe it is however a shortcoming in Tokio's functionality of bidirectional copy. That or I'm just not using it correctly.

Created a bug for it at tokio-rs/tokio#4674.

[GlenDC]

For now I forked for my project the 2 util files from Tokio's copy bidirectional code. With minor modifications I can make it work by catching those 2 error kinds that can happen during MacOS shutdown of an I/O stream. My modified code is posted in the linked Tokio issue.

Seems the responsibility is entirely to be found within Tokio, or even Rust's std lib. Or at the very least the responsibility of needing to deal with a problem within MacOS.