Glide-core UDS Socket Handling Rework

[ikolomi]

1.Introduced a user-land mechanism for ensuring singleton behavior of the socket, rather than relying on OS-specific semantics. This addresses the issue where macOS and Linux report different errors when the socket path already exists.

2.Simplified the implementation by removing unnecessary abstractions, including redundant connection retry logic.

Issue link

This Pull Request is linked to issue (URL): [https://github.com//issues/2433]

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message has a detailed description of what changed and why.
• Tests are added or updated.
• CHANGELOG.md and documentation files are updated.
• Destination branch is correct - main or release
• Commits will be squashed upon merging.

[eifrah-aws]

why the Arc?

[ikolomi]

I thought it is needed because the lock is shared between the threads. Will check

[eifrah-aws]

Question:

Can we have more than 1 socket? (looking at the socket path it is hard coded to:

let socket_name = format!("{}-{}", SOCKET_FILE_NAME, std::process::id());

So it seems 1 per process (unless it is needed for testing purposes)

[ikolomi]

its odd but multiple calls with different socket_path are allowed, leading to multiple UDS sockets

[eifrah-aws]

I don't think its needed since this method is mostly called once in the lifetime of a client creation

[ikolomi]

this is not true. multiple sockets are allowed (dont want to change it )

[eifrah-aws]

Suggested change

	let runtime = Builder::new_current_thread().enable_all().build();
	if let Err(err) = runtime {
	log_error(
	"listen_on_socket",
	format!("Error failed to create a new tokio thread: {err}"),
	);
	return Err(err);
	}
	let runtime = match Builder::new_current_thread().enable_all().build() {
	Err(err) => {
	log_error(
	"listen_on_socket",
	format!("Error failed to create a new tokio thread: {err}"),
	);
	return Err(err);
	}
	Ok(runtime) => runtime,
	};

[ikolomi]

i dont see why its better

[eifrah-aws]

Should you accept my suggestion above, we can drop the unwrap() here

[ikolomi]

i think your suggestion is a less elegant. unwrap() is legit, otherwise it would not be in the language or clippy would complain

[eifrah-aws]

Suggested change

	let listener_socket = UnixListener::bind(socket_path_cloned.clone());
	if let Err(err) = listener_socket {
	log_error(
	"listen_on_socket",
	format!("Error failed to bind listening socket: {err}"),
	);
	return Err(err);
	}
	let listener_socket = listener_socket.unwrap();
	let listener_socket = match UnixListener::bind(socket_path_cloned.clone()) {
	Err(err) ==> {
	log_error(
	"listen_on_socket",
	format!("Error failed to bind listening socket: {err}"),
	);
	return Err(err);
	}
	Ok(listener_socket) => listener_socket,
	};

[ikolomi]

same as above

[eifrah-aws]

The only reason a tx.send will fail means that the receiving end has dropped - we should handle this

[ikolomi]

what do you mean by handle ? we could log maybe, but its so unimportant I would not bother
Otherwise, it is safer to process to the accept loop since it might serve other clients

[eifrah-aws]

This function should return a Result. This will reduce the code below by 1/2

[eifrah-aws]

You should consider making this method returning a Result.

This highlighted lines above can then become:

let stream = listener_socket.accept().await?.0;
local_set_pool.spawn_pinned(move || listen_on_client_stream(stream));

[ikolomi]

isnt having an await? will make it impossible to log "error accepting connection"

[barshaul]

If a new client will be created between the time you drop the listener_socket/when we break from the accept loop to the time we remove it from the sockets_write_guard, the new client would get that there's an existing socket listener and return its path although it isn't available anymore. how do we handle that? it would probably make the wrapper to fail trying to connect to a bad socket path

[ikolomi]

I think it does not differ from the previous behavior, which I find odd, but I do not want to change the design at this time.
The weak point of the design is that the init callback is called upon the socket creation. Thus, if the loop terminates by failing to accept the connection - the callback might be called with a success but the connection will not be accepted.

With the previous implementation what you are describing might happen like this:

1. client A is created, started thread_a, binding and accepting on the socket
2. client B creation started, creating thread_b, which detects an existing socket which he could connect to
3. thread_a failed to accept the connection and terminated
4. client B init callback called with a success, thread_b does not continue (since it detected a connectable socket) but the creation eventually fails since the connection cannot be established

To summarize - the new implementation does not degrade, but even improves the situation by having the explicit remove_file and user-land socket db update upon the accept loop termination (in the orig implementation the socket file will remain causing the following creations to fail)

[Yury-Fridlyand]

Add a changelog please