workers-rs
workers-rs copied to clipboard
[BUG] TCP Connect does not error if connection not able to be created
Is there an existing issue for this?
- [X] I have searched the existing issues
What version of workers-rs
are you using?
main
What version of wrangler
are you using?
3.26.0
Describe the bug
The tldr of this issue is a request to make connect() fail if the connection could not be established due to a closed port, or some other reason. See below for details
It seems that socket connect()
doesn't actually return an error if a connection is unable to be created for some reason (port closed, etc). Trying to build a worker that as quickly and simply as possible can let a user know if a port on their IP is open or not.
Generally, I'd expect an error from the connect() call if the port is closed, but in the workers env, this doesn't seem to hold true. Here's the code that I would expect to fail on a closed port that just continues past a closed port:
let mut socket = Socket::builder()
.secure_transport(SecureTransport::Off)
.connect(&connecting_ip, port)
.map_err(|_| worker::Error::from("Could not create socket"))?;
For the most part, I can reliably tell if the port is open or closed by attempting to write some data AND then read a byte from the socket after the connect call (write isn't enough, and also doesn't seem to fail if the port is closed - have to add the read step). This isn't particularly ideal though, since I have to rely on binding setTimeout
into rust and setting a timeout on the read to determine if its not going to succeed. This method also doesn't work for all protocols that I need to check for open ports.
I was curious to see how this worked in a non-workers rust environment, and was able to reliably determine if a port is open at the connect call, using the following code:
use std::net::{SocketAddr, TcpStream};
use std::str::FromStr;
use std::time::Duration;
fn main() {
let remote_ip = "1.2.3.4";
let port = 123;
let timeout = Duration::from_secs(1);
// Construct the remote address from the IP and port
let remote_addr = match SocketAddr::from_str(&format!("{}:{}", remote_ip, port)) {
Ok(addr) => addr,
Err(e) => {
eprintln!("Error constructing remote address: {}", e);
return;
}
};
// Attempt to connect with a timeout
match TcpStream::connect_timeout(&remote_addr, timeout) {
Ok(_) => println!("Port {} is open on {}", port, remote_ip),
Err(e) => println!("Failed to connect to port {} on {}: {}", port, remote_ip, e),
}
}
Steps To Reproduce
No response
This matches the behavior of the JavaScript API, which returns from connect
immediately (it is not a Promise
), and thus does not indicate whether the connection has been established with the remote server. Errors are then propagated when trying to read / write from the socket.
This matches the behavior of the JavaScript API, which returns from
connect
immediately (it is not aPromise
), and thus does not indicate whether the connection has been established with the remote server. Errors are then propagated when trying to read / write from the socket.
So thats generally fine, but I was also seeing a write not fail on a socket that didn't actually have an open port, unless I did this wrong somehow.
let mut socket = Socket::builder()
.secure_transport(SecureTransport::Off)
.connect(&connecting_ip, port)
.map_err(|_| worker::Error::from("Could not create socket"))?;
socket
.write(b"GET / HTTP/1.0\r\nHost: example.com\r\n\r\n")
.await
.map_err(|_| worker::Error::from("Could not write to socket"))?;
This write doesn't fail for me, even on a closed port, and it sounds like it should err?
Does reading return an error? I think it should error, but I will have to check the JavaScript behavior.
Reading returns an error in most cases. There are certain types of applications where this doesn't seem to be particularly reliable though.
I did some testing in node earlier, and while the socket.connect call itself does not return a promise, you are able to attach event listeners to events like connect
, timeout
, and error
which enable getting at this info more reliably than writing then trying to read. It would be nice to have some way to get at this in Rust (and I was tinkering a bit in JS with workers, and wasn't able to get it to work in that context either). Node test code here https://gist.github.com/cmmarslender/145a929163d8f3501490bf00bed2e993
I think my current rust implementation covers most of the cases I need it to cover, but it would be nice if this could eventually work with just connect and an error there, or attaching an on connect/error/timeout handler, or perhaps some other solution, that doesn't require having to write and read.
@dom96 thoughts on this behavior?
I did some testing in node earlier, and while the socket.connect call itself does not return a promise, you are able to attach event listeners to events like connect, timeout, and error which enable getting at this info more reliably than writing then trying to read
We now have an opened
promise that can be used for this purpose. It should get rejected if the connection fails to be opened and that shouldn't require any reads/writes to happen.
As for the write
not failing, I believe that is expected (for the same reason connect
doesn't return a promise which waits for the connection result) and the error ends up in the closed
promise instead.
I did some testing in node earlier, and while the socket.connect call itself does not return a promise, you are able to attach event listeners to events like connect, timeout, and error which enable getting at this info more reliably than writing then trying to read
We now have an
opened
promise that can be used for this purpose. It should get rejected if the connection fails to be opened and that shouldn't require any reads/writes to happen.As for the
write
not failing, I believe that is expected (for the same reasonconnect
doesn't return a promise which waits for the connection result) and the error ends up in theclosed
promise instead.
is it possible to get at the opened
promise in some way from Rust, and if so, are there docs or an example on this somewhere? I haven't seen anything, but may be looking in the wrong places.
Yep, I think it would be reasonable to add that to the Rust API. Also looks like we need to add it to our JavaScript docs.