Fix client trying to use “occupied file” for socket. #3200

@@ -73,10 +73,10 @@ native-tls = { version = "0.2", optional = true }
 tokio-native-tls = { version = "0.3", optional = true }

 # Only needed for rustls
-rustls = { version = "0.22", optional = true }
+rustls = { version = "0.23.23", optional = true }


any reason we are locked into a specific minor version?

I actually tried to do the opposite everywhere, mistake

eifrah-aws · 2025-03-02T05:34:03Z

glide-core/redis-rs/redis/Cargo.toml

@@ -160,13 +160,13 @@ disable-client-setinfo = []
 tls = ["tls-native-tls"] # use "tls-native-tls" instead

 [dev-dependencies]
-rand = "0.8"
+rand = "0.9"


imo, we should just use major version, e.g:

rand = "0"

When its not stable yet, minor can be breaking as well, that's why under one i left the minor

eifrah-aws · 2025-03-02T05:35:22Z

glide-core/redis-rs/redis/src/connection.rs

-        root_store.add(cert)?;
+    let native_certs = load_native_certs();
+    #[cfg(all(
+        feature = "tls-rustls",


Do we support both versions? if so, why?

eifrah-aws · 2025-03-02T05:36:33Z

glide-core/redis-rs/rustfmt.toml

@@ -1,2 +1,2 @@
 use_try_shorthand = true
-edition = "2018"
+edition = "2024"


will this break on older versions of Ubuntu, for example 22.04 LTS? (I am not sure which version of Rust they provide, but this might be a concern)

we can also use 2021 if 2018 does not work with our current code base

Break for usage or break for development?

eifrah-aws · 2025-03-05T06:06:48Z

glide-core/src/glide_paths.rs

+}
+
+/// For ergonomics: Converts a string into a PathBuf.
+fn string_to_pathbuf(string: &str) -> PathBuf {


How is this shorter than writing PathBuf::from ?

eifrah-aws · 2025-03-05T06:14:39Z

glide-core/src/glide_paths.rs

+    pub glide_file: PathBuf,
+}
+/// GlidePaths struct implementation.
+impl GlidePaths {


When I suggested using a GlidePaths struct, I was talking in general. However, your code always refers to the socket file. The struct should be for "general purpose" for example, once stabilized, we should use it with other file related components (for example, a global location where we want to place our logs)

Your comment was actually leading to the other direction 😅 i tried to still keep it a bit generic.

Let's put it this way -

Want generic file struct

I want it to be handy for, for example, doing socket file tasks. Meaning i do want the struct to have some special sockets functionality in addition.

I don't want to create a whole component which is not needed yet. More like laying foundation, and when needed can be enlarged.

eifrah-aws · 2025-03-05T06:15:28Z

glide-core/src/glide_paths.rs

+    }
+
+    /// Create a new GlidePaths struct from string paths.
+    pub fn from_file_path(glide_file: &str) -> Self {


Why is this needed?

eifrah-aws · 2025-03-05T06:15:38Z

glide-core/src/glide_paths.rs

+    }
+
+    /// Create a new GlidePaths struct from string file name.
+    pub fn from_file_name(file_name: &str) -> Self {


Why is this needed?

This is how tests were built. The old implementation also had name options for tests.
I can remove it, but will need to refactor tests

eifrah-aws · 2025-03-05T06:20:21Z

glide-core/src/glide_paths.rs

+
+    /// Returns the path to the temporary directory where the socket file is stored.
+    fn glide_sock_dir_path() -> PathBuf {
+        let path = std::env::temp_dir()


GlidePaths should have common API, like:

impl GlidePaths { pub fn initialise() -> Result<(), io::Error> { // Create the directory structures once } pub fn temp_dir() -> Option<&PathBuf> { } pub fn logs_dir() -> Option<&PathBuf> { } pub fn socket_file() -> Option<&PathBuf> { /// Append the socket file name to the `Self::tmep_dir()` } }

avifenesh requested a review from a team as a code owner February 18, 2025 14:32

avifenesh changed the title ~~Update dependencies and refactor random string generation~~ Fix client trying to use “occupied file” for socket. Feb 18, 2025

avifenesh force-pushed the clean_socket_file_on_connect branch 2 times, most recently from 3451846 to 4c00879 Compare February 18, 2025 14:41

avifenesh marked this pull request as draft February 18, 2025 14:49

avifenesh force-pushed the clean_socket_file_on_connect branch 4 times, most recently from d793a2d to cf77e7d Compare February 20, 2025 02:13

avifenesh marked this pull request as ready for review February 20, 2025 02:38

avifenesh requested review from barshaul, ikolomi and eifrah-aws February 20, 2025 16:32

eifrah-aws requested changes Feb 20, 2025

View reviewed changes

avifenesh removed request for barshaul and ikolomi February 20, 2025 18:01

avifenesh force-pushed the clean_socket_file_on_connect branch 6 times, most recently from bbd633d to 4d5983f Compare February 22, 2025 23:59

avifenesh requested a review from eifrah-aws February 23, 2025 02:25

This was linked to issues Feb 23, 2025

[Task] Get rid of ring dependency in core #3226

Open

Node/Docker: failed to bind listening socket #2985

Open

Yury-Fridlyand reviewed Feb 24, 2025

View reviewed changes

glide-core/redis-rs/redis/src/connection.rs Show resolved Hide resolved

glide-core/Cargo.toml Show resolved Hide resolved

avifenesh self-assigned this Feb 24, 2025

avifenesh force-pushed the clean_socket_file_on_connect branch 2 times, most recently from 3557cbd to 2b77a59 Compare March 1, 2025 11:29

bump and revise versions

898039f

Signed-off-by: avifenesh <[email protected]>

avifenesh added 2 commits March 1, 2025 12:02

fix ring dependency

75e6ea4

Signed-off-by: avifenesh <[email protected]>

Refactor random number generation to use updated rand crate methods a…

58a537c

…nd add glide_paths module Signed-off-by: avifenesh <[email protected]>

avifenesh force-pushed the clean_socket_file_on_connect branch from 2b77a59 to 6e38e4c Compare March 1, 2025 12:16

avifenesh requested a review from Yury-Fridlyand March 1, 2025 12:16

Update dependencies and improve error handling in connection module

f9a09b0

Signed-off-by: avifenesh <[email protected]>

avifenesh force-pushed the clean_socket_file_on_connect branch from 6e38e4c to f9a09b0 Compare March 1, 2025 12:25

Fix type assertion for Buffer conversion in BaseClient and GlideClust…

164a408

…erClient Signed-off-by: avifenesh <[email protected]>

avifenesh force-pushed the clean_socket_file_on_connect branch from 0131ecf to 164a408 Compare March 1, 2025 12:46

avifenesh added 2 commits March 5, 2025 00:06

Merge branch 'main' into clean_socket_file_on_connect

049d4e8

Signed-off-by: Avi Fenesh <[email protected]>

Merge branch 'main' into clean_socket_file_on_connect

433a1f0

eifrah-aws requested changes Mar 5, 2025

View reviewed changes

Merge branch 'main' into clean_socket_file_on_connect

ddc3074

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix client trying to use “occupied file” for socket. #3200

Fix client trying to use “occupied file” for socket. #3200

avifenesh commented Feb 18, 2025 •

edited

Loading

eifrah-aws Mar 2, 2025

avifenesh Mar 5, 2025

eifrah-aws Mar 2, 2025

avifenesh Mar 5, 2025

eifrah-aws Mar 2, 2025

eifrah-aws Mar 2, 2025

eifrah-aws Mar 5, 2025

avifenesh Mar 5, 2025

eifrah-aws Mar 5, 2025

eifrah-aws Mar 5, 2025

avifenesh Mar 5, 2025

eifrah-aws Mar 5, 2025

eifrah-aws Mar 5, 2025

avifenesh Mar 5, 2025

eifrah-aws Mar 5, 2025

Fix client trying to use “occupied file” for socket. #3200

Are you sure you want to change the base?

Fix client trying to use “occupied file” for socket. #3200

Conversation

avifenesh commented Feb 18, 2025 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avifenesh commented Feb 18, 2025 •

edited

Loading