Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Rust 2021, and auto-apply clippy lints #148

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update to Rust 2021, and auto-apply clippy lints #148

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15fb25e
Update to Rust 2021, and auto-apply clippy lints
Swatinem Apr 11, 2024
40baf21
Use more idiomatic Rust
Swatinem Apr 11, 2024
cb8fe16
Avoid intermediate `HashMap` in `gets`
Swatinem Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ readme = "README.md"
license = "MIT"
description = "memcached client for rust"
keywords = ["memcache", "memcached", "driver", "cache", "database"]
edition = "2018"
edition = "2021"

[features]
default = ["tls"]
Expand Down
30 changes: 18 additions & 12 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ impl Client {
let parsed = Url::parse(url.as_str())?;
let timeout = parsed
.query_pairs()
.find(|&(ref k, ref _v)| k == "connect_timeout")
.find(|(k, _v)| k == "connect_timeout")
.and_then(|(ref _k, ref v)| v.parse::<f64>().ok())
.map(Duration::from_secs_f64);
let builder = r2d2::Pool::builder().max_size(size);
Expand Down Expand Up @@ -256,7 +256,7 @@ impl Client {

for key in keys {
let connection_index = (self.hash_function)(key) as usize % connections_count;
let array = con_keys.entry(connection_index).or_insert_with(Vec::new);
let array = con_keys.entry(connection_index).or_default();
array.push(key);
}
for (&connection_index, keys) in con_keys.iter() {
Expand Down Expand Up @@ -467,6 +467,12 @@ pub struct ClientBuilder {
hash_function: fn(&str) -> u64,
}

impl Default for ClientBuilder {
fn default() -> Self {
Self::new()
}
}

impl ClientBuilder {
/// Create an empty client builder.
pub fn new() -> Self {
Expand All @@ -486,7 +492,7 @@ impl ClientBuilder {
pub fn add_server<C: Connectable>(mut self, target: C) -> Result<Self, MemcacheError> {
let targets = target.get_urls();

if targets.len() == 0 {
if targets.is_empty() {
return Err(MemcacheError::BadURL("No servers specified".to_string()));
}

Expand Down Expand Up @@ -540,7 +546,7 @@ impl ClientBuilder {
pub fn build(self) -> Result<Client, MemcacheError> {
let urls = self.targets;

if urls.len() == 0 {
if urls.is_empty() {
return Err(MemcacheError::BadURL("No servers specified".to_string()));
}

Expand Down Expand Up @@ -572,7 +578,7 @@ impl ClientBuilder {

let connection = builder
.build(ConnectionManager::new(url))
.map_err(|e| MemcacheError::PoolError(e))?;
.map_err(MemcacheError::PoolError)?;

connections.push(connection);
}
Expand Down Expand Up @@ -600,7 +606,7 @@ mod tests {
.unwrap()
.build()
.unwrap();
assert!(client.version().unwrap()[0].1 != "");
assert!(!client.version().unwrap()[0].1.is_empty());
}

#[test]
Expand Down Expand Up @@ -710,14 +716,14 @@ mod tests {
#[test]
fn unix() {
let client = super::Client::connect("memcache:///tmp/memcached.sock").unwrap();
assert!(client.version().unwrap()[0].1 != "");
assert!(!client.version().unwrap()[0].1.is_empty());
}

#[cfg(feature = "tls")]
#[test]
fn ssl_noverify() {
let client = super::Client::connect("memcache+tls://localhost:12350?verify_mode=none").unwrap();
assert!(client.version().unwrap()[0].1 != "");
assert!(!client.version().unwrap()[0].1.is_empty());
}

#[cfg(feature = "tls")]
Expand All @@ -726,22 +732,22 @@ mod tests {
let client =
super::Client::connect("memcache+tls://localhost:12350?ca_path=tests/assets/RUST_MEMCACHE_TEST_CERT.crt")
.unwrap();
assert!(client.version().unwrap()[0].1 != "");
assert!(!client.version().unwrap()[0].1.is_empty());
}

#[cfg(feature = "tls")]
#[test]
fn ssl_client_certs() {
let client = super::Client::connect("memcache+tls://localhost:12351?key_path=tests/assets/client.key&cert_path=tests/assets/client.crt&ca_path=tests/assets/RUST_MEMCACHE_TEST_CERT.crt").unwrap();
assert!(client.version().unwrap()[0].1 != "");
assert!(!client.version().unwrap()[0].1.is_empty());
}

#[test]
fn delete() {
let client = super::Client::connect("memcache://localhost:12345").unwrap();
client.set("an_exists_key", "value", 0).unwrap();
assert_eq!(client.delete("an_exists_key").unwrap(), true);
assert_eq!(client.delete("a_not_exists_key").unwrap(), false);
assert!(client.delete("an_exists_key").unwrap());
assert!(!client.delete("a_not_exists_key").unwrap());
}

#[test]
Expand Down
34 changes: 14 additions & 20 deletions src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,13 @@ struct TcpOptions {

#[cfg(feature = "tls")]
fn get_param(url: &Url, key: &str) -> Option<String> {
return url
.query_pairs()
.find(|&(ref k, ref _v)| k == key)
.map(|(_k, v)| v.to_string());
url.query_pairs().find(|(k, _v)| k == key).map(|(_k, v)| v.to_string())
}

#[cfg(feature = "tls")]
impl TlsOptions {
fn from_url(url: &Url) -> Result<Self, MemcacheError> {
let verify_mode = match get_param(url, "verify_mode").as_ref().map(String::as_str) {
let verify_mode = match get_param(url, "verify_mode").as_deref() {
Some("none") => SslVerifyMode::NONE,
Some("peer") => SslVerifyMode::PEER,
Some(_) => {
Expand All @@ -132,10 +129,10 @@ impl TlsOptions {

Ok(TlsOptions {
tcp_options: TcpOptions::from_url(url),
ca_path: ca_path,
key_path: key_path,
cert_path: cert_path,
verify_mode: verify_mode,
ca_path,
key_path,
cert_path,
verify_mode,
})
}
}
Expand All @@ -147,21 +144,18 @@ impl TcpOptions {
.any(|(ref k, ref v)| k == "tcp_nodelay" && v == "false");
let timeout = url
.query_pairs()
.find(|&(ref k, ref _v)| k == "timeout")
.find(|(k, _v)| k == "timeout")
.and_then(|(ref _k, ref v)| v.parse::<f64>().ok())
.map(Duration::from_secs_f64);
TcpOptions {
nodelay: nodelay,
timeout: timeout,
}
TcpOptions { nodelay, timeout }
}
}

impl Transport {
fn from_url(url: &Url) -> Result<Self, MemcacheError> {
let mut parts = url.scheme().splitn(2, "+");
let mut parts = url.scheme().splitn(2, '+');
match parts.next() {
Some(part) if part == "memcache" => (),
Some("memcache") => (),
_ => {
return Err(MemcacheError::BadURL(
"memcache URL's scheme should start with 'memcache'".into(),
Expand Down Expand Up @@ -191,7 +185,7 @@ impl Transport {

#[cfg(unix)]
{
if url.host().is_none() && url.port() == None {
if url.host().is_none() && url.port().is_none() {
return Ok(Transport::Unix);
}
}
Expand Down Expand Up @@ -254,12 +248,12 @@ impl Connection {
let protocol = if is_ascii {
Protocol::Ascii(AsciiProtocol::new(stream))
} else {
Protocol::Binary(BinaryProtocol { stream: stream })
Protocol::Binary(BinaryProtocol { stream })
};

Ok(Connection {
url: Arc::new(url.to_string()),
protocol: protocol,
protocol,
})
}
}
Expand All @@ -273,7 +267,7 @@ mod tests {
use url::Url;
match Transport::from_url(&Url::parse("memcache:///tmp/memcached.sock").unwrap()).unwrap() {
Transport::Unix => (),
_ => assert!(false, "transport is not unix"),
_ => panic!("transport is not unix"),
}
}
}
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ assert_eq!(answer, 42);
```
!*/

#![cfg_attr(feature = "cargo-clippy", allow(clippy::needless_return))]
#![allow(clippy::needless_return)]

extern crate byteorder;
extern crate enum_dispatch;
Expand Down
39 changes: 17 additions & 22 deletions src/protocol/ascii.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use std::borrow::Cow;
pub struct Options {
pub noreply: bool,
pub exptime: u32,
pub flags: u32,
pub cas: Option<u64>,
}

Expand All @@ -27,7 +26,7 @@ enum StoreCommand {
Prepend,
}

const END: &'static str = "END\r\n";
const END: &str = "END\r\n";

impl fmt::Display for StoreCommand {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Expand All @@ -45,15 +44,13 @@ impl fmt::Display for StoreCommand {
struct CappedLineReader<C> {
inner: C,
filled: usize,
buf: [u8; 2048],
buf: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The extracted content from the test test_capped_line_reader in the src/protocol/ascii.rs file shows that the test primarily focuses on the functionality of reading lines correctly using the CappedLineReader. The test uses a mock reader to simulate input and checks if the CappedLineReader can correctly parse lines from the input. However, there is no indication that this test assesses memory usage or performance implications of using a Vec<u8> instead of a fixed-size array.

Given the lack of specific tests or benchmarks addressing the memory usage or performance after changing to Vec<u8>, we cannot conclusively verify that there are no unintended side effects such as increased memory usage or performance degradation due to this change. It would be advisable to implement or look for additional benchmarks or profiling tests that specifically measure these aspects.

Analysis chain

Replacing fixed-size array with Vec<u8> in CappedLineReader struct allows for more flexible buffer management. Ensure that there are no unintended side effects such as increased memory usage or performance degradation.

The search results from the rg command indicate that the CappedLineReader struct is indeed present in the src/protocol/ascii.rs file and is used in various contexts, including instantiation and method implementations. However, the output does not provide direct evidence regarding the impact of replacing a fixed-size array with Vec<u8> on memory usage or performance.

To further verify the impact of this change, we need to examine the usage patterns of CappedLineReader and possibly look for any benchmarks or tests that measure memory usage or performance. This will help us determine if there are any reported or observable issues related to the change.


---

The search results indicate that there is a test specifically for the `CappedLineReader` within the `src/protocol/ascii.rs` file. This suggests that there is some level of automated testing in place to verify the functionality of the `CappedLineReader`. However, the output does not provide details on whether this test covers aspects related to memory usage or performance, which are critical in assessing the impact of changing from a fixed-size array to a `Vec<u8>`.

To conclusively verify the impact of this change on memory usage and performance, we need to examine the contents of the test to see if it addresses these concerns.
Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any reported issues with increased memory usage or performance degradation due to this change.
rg --type rust "CappedLineReader"

Length of output: 365

Script:

#!/bin/bash
# Search for benchmarks or tests related to CappedLineReader
rg --type rust "CappedLineReader" -A 30 -B 30 | rg "bench|test"

Length of output: 223

Script:

#!/bin/bash
# Extract the content of the test for CappedLineReader to assess its focus areas
rg --type rust --context 10 "fn test_capped_line_reader" src/protocol/ascii.rs

Length of output: 786

}

fn get_line(buf: &[u8]) -> Option<usize> {
for (i, r) in buf.iter().enumerate() {
if *r == b'\r' {
if buf.get(i + 1) == Some(&b'\n') {
return Some(i + 2);
}
if *r == b'\r' && buf.get(i + 1) == Some(&b'\n') {
return Some(i + 2);
}
}
None
Expand All @@ -64,7 +61,7 @@ impl<C: Read> CappedLineReader<C> {
Self {
inner,
filled: 0,
buf: [0x0; 2048],
buf: vec![0x0; 2048],
}
}

Expand All @@ -77,7 +74,7 @@ impl<C: Read> CappedLineReader<C> {
let (to_fill, rest) = buf.split_at_mut(min);
to_fill.copy_from_slice(&self.buf[..min]);
self.consume(min);
if rest.len() != 0 {
if !rest.is_empty() {
self.inner.read_exact(&mut rest[..])?;
}
Ok(())
Expand All @@ -98,7 +95,7 @@ impl<C: Read> CappedLineReader<C> {
}
loop {
let (_filled, buf) = self.buf.split_at_mut(self.filled);
if buf.len() == 0 {
if buf.is_empty() {
return Err(ClientError::Error(Cow::Borrowed("Ascii protocol response too long")))?;
}
let read = self.inner.read(&mut buf[..])?;
Expand Down Expand Up @@ -131,7 +128,7 @@ impl ProtocolTrait for AsciiProtocol<Stream> {
}

fn version(&mut self) -> Result<String, MemcacheError> {
self.reader.get_mut().write(b"version\r\n")?;
self.reader.get_mut().write_all(b"version\r\n")?;
self.reader.get_mut().flush()?;
self.reader.read_line(|response| {
let response = MemcacheError::try_from(response)?;
Expand Down Expand Up @@ -294,7 +291,7 @@ impl ProtocolTrait for AsciiProtocol<Stream> {
}

fn stats(&mut self) -> Result<Stats, MemcacheError> {
self.reader.get_mut().write(b"stats\r\n")?;
self.reader.get_mut().write_all(b"stats\r\n")?;
self.reader.get_mut().flush()?;

enum Loop {
Expand All @@ -312,7 +309,7 @@ impl ProtocolTrait for AsciiProtocol<Stream> {
if !s.starts_with("STAT") {
return Err(ServerError::BadResponse(Cow::Owned(s.into())))?;
}
let stat: Vec<_> = s.trim_end_matches("\r\n").split(" ").collect();
let stat: Vec<_> = s.trim_end_matches("\r\n").split(' ').collect();
if stat.len() < 3 {
return Err(ServerError::BadResponse(Cow::Owned(s.into())).into());
}
Expand Down Expand Up @@ -348,12 +345,10 @@ impl AsciiProtocol<Stream> {
value: V,
options: &Options,
) -> Result<bool, MemcacheError> {
if command == StoreCommand::Cas {
if options.cas.is_none() {
Err(ClientError::Error(Cow::Borrowed(
"cas_id should be present when using cas command",
)))?;
}
if command == StoreCommand::Cas && options.cas.is_none() {
Err(ClientError::Error(Cow::Borrowed(
"cas_id should be present when using cas command",
)))?;
}
let noreply = if options.noreply { " noreply" } else { "" };
if options.cas.is_some() {
Comment on lines 345 to 354
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The verification process did not find any test cases specifically covering the store method or CAS operations in the provided codebase. It is recommended to add comprehensive tests to ensure the functionality and error handling of the store method, especially for edge cases involving CAS operations, are well-tested.

Analysis chain

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [348-380]

The changes in the store method, including the check for cas_id presence and the use of write_all, improve error handling and data integrity. Ensure that these changes are well-tested, especially in edge cases.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify comprehensive testing of the `store` method, particularly for edge cases involving CAS operations.
rg --type rust "store.*test"

Length of output: 30

Script:

#!/bin/bash
# Broaden the search to find any test cases related to the `store` method or CAS operations in the test files.
rg --type rust --files-with-matches 'fn test' | xargs rg 'store|CAS'

Length of output: 703

Expand Down Expand Up @@ -382,7 +377,7 @@ impl AsciiProtocol<Stream> {
}

value.write_to(self.reader.get_mut())?;
self.reader.get_mut().write(b"\r\n")?;
self.reader.get_mut().write_all(b"\r\n")?;
self.reader.get_mut().flush()?;

if options.noreply {
Expand Down Expand Up @@ -424,7 +419,7 @@ impl AsciiProtocol<Stream> {
if !buf.starts_with("VALUE") {
return Err(ServerError::BadResponse(Cow::Owned(buf.into())))?;
}
let mut header = buf.trim_end_matches("\r\n").split(" ");
let mut header = buf.trim_end_matches("\r\n").split(' ');
let mut next_or_err = || {
header
.next()
Expand Down Expand Up @@ -481,7 +476,7 @@ mod tests {
match self.reads.pop_front() {
Some(range) => {
let range = &self.data[range];
(&mut buf[0..range.len()]).copy_from_slice(&range);
buf[0..range.len()].copy_from_slice(range);
Ok(range.len())
}
None => Err(std::io::ErrorKind::WouldBlock.into()),
Expand Down
Loading