feat: re-enable offset optimization

NOTE: During work on this patch, the application faulted very early in processing. This may have been due to a library issue around libssl setting an `_atexit()` handler that conflicts with the rust threading model. (see sfackler/rust-openssl#1174 and openssl/openssl#6214 for possibly related issues) Closes #645
mozilla-services · Jul 15, 2020 · e28ae7b · e28ae7b
1 parent 9465952
commit e28ae7b
Show file tree

Hide file tree

Showing 24 changed files with 2,535 additions and 2,988 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -42,7 +42,7 @@ mime = "0.3"
 mozsvc-common = "0.1"
 num_cpus = "1"
 # must match what's used by googleapis-raw
-protobuf = "2.15"
+protobuf = "2.16.2"
 rand = "0.7"
 regex = "1.3"
 sentry = { version = "0.18", features = ["with_curl_transport"] }

diff --git a/src/db/mysql/models.rs b/src/db/mysql/models.rs
@@ -507,13 +507,29 @@ impl MysqlDb {
         // match the query conditions
         query = query.limit(if limit >= 0 { limit + 1 } else { limit });
 
-        let numeric_offset = offset.map_or(0, |offset| offset.offset as i64);
-
-        if numeric_offset != 0 {
-            // XXX: copy over this optimization:
-            // https://github.com/mozilla-services/server-syncstorage/blob/a0f8117/syncstorage/storage/sql/__init__.py#L404
-            query = query.offset(numeric_offset);
+        let mut numeric_offset = 0;
+
+        if let Some(offset) = offset {
+            numeric_offset = offset.offset as i64;
+            query = match sort {
+                Sorting::Index | Sorting::None => query.offset(numeric_offset),
+                Sorting::Newest => {
+                    if let Some(timestamp) = offset.timestamp {
+                        query.filter(bso::modified.le(timestamp.as_i64()))
+                    } else {
+                        query
+                    }
+                }
+                Sorting::Oldest => {
+                    if let Some(timestamp) = offset.timestamp {
+                        query.filter(bso::modified.ge(timestamp.as_i64()))
+                    } else {
+                        query
+                    }
+                }
+            }
         }
+
         let mut bsos = query.load::<results::GetBso>(&self.conn)?;
 
         // XXX: an additional get_collection_timestamp is done here in
@@ -522,8 +538,17 @@ impl MysqlDb {
         //}
 
         let next_offset = if limit >= 0 && bsos.len() > limit as usize {
-            bsos.pop();
-            Some((limit + numeric_offset).to_string())
+            if let Some(last) = bsos.pop() {
+                let start_next = limit + numeric_offset;
+                match sort {
+                    Sorting::Index | Sorting::None => Some(start_next.to_string()),
+                    Sorting::Newest | Sorting::Oldest => {
+                        Some(format!("{}:{}", last.modified.as_i64(), start_next))
+                    }
+                }
+            } else {
+                None
+            }
         } else {
             None
         };
@@ -578,11 +603,17 @@ impl MysqlDb {
         query = query.limit(if limit >= 0 { limit + 1 } else { limit });
 
         let numeric_offset = offset.map_or(0, |offset| offset.offset as i64);
-        if numeric_offset != 0 {
-            // XXX: copy over this optimization:
-            // https://github.com/mozilla-services/server-syncstorage/blob/a0f8117/syncstorage/storage/sql/__init__.py#L404
-            query = query.offset(numeric_offset);
-        }
+
+        query = if numeric_offset != 0 {
+            match sort {
+                Sorting::Index => query.offset(numeric_offset),
+                Sorting::Newest => query.filter(bso::modified.gt(numeric_offset)),
+                Sorting::Oldest => query.filter(bso::modified.lt(numeric_offset)),
+                _ => query,
+            }
+        } else {
+            query
+        };
         let mut ids = query.load::<String>(&self.conn)?;
 
         // XXX: an additional get_collection_timestamp is done here in

diff --git a/src/db/tests/db.rs b/src/db/tests/db.rs
@@ -180,6 +180,7 @@ async fn get_bsos_limit_offset() -> Result<()> {
     assert_eq!(bsos.items[0].id, "11");
     assert_eq!(bsos.items[4].id, "7");
 
+    let offset = bsos.offset.unwrap();
     let bsos2 = db
         .get_bsos(gbsos(
             uid,
@@ -189,7 +190,7 @@ async fn get_bsos_limit_offset() -> Result<()> {
             newer,
             Sorting::Newest,
             limit,
-            &bsos.offset.unwrap(),
+            &offset,
         ))
         .await?;
     assert_eq!(bsos2.items.len(), 5 as usize);

diff --git a/src/main.rs b/src/main.rs
@@ -25,6 +25,7 @@ struct Args {
 
 #[actix_rt::main]
 async fn main() -> Result<(), Box<dyn Error>> {
+    println!("Go...");
     let args: Args = Docopt::new(USAGE)
         .and_then(|d| d.deserialize())
         .unwrap_or_else(|e| e.exit());

diff --git a/src/web/extractors.rs b/src/web/extractors.rs
@@ -1227,13 +1227,6 @@ impl ToString for Offset {
 impl FromStr for Offset {
     type Err = ParseIntError;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        // issue559: Disable ':' support for now: simply parse as i64 as
-        // previously (it was u64 previously but i64's close enough)
-        let result = Offset {
-            timestamp: None,
-            offset: s.parse::<u64>()?,
-        };
-        /*
         let result = match s.chars().position(|c| c == ':') {
             None => Offset {
                 timestamp: None,
@@ -1250,7 +1243,6 @@ impl FromStr for Offset {
                 }
             }
         };
-        */
         Ok(result)
     }
 }

diff --git a/vendor/mozilla-rust-sdk/googleapis-raw/Cargo.toml b/vendor/mozilla-rust-sdk/googleapis-raw/Cargo.toml
@@ -7,7 +7,7 @@ edition = "2018"
 [dependencies]
 futures = "0.3.5"
 grpcio = "0.6.0"
-protobuf = "2.15.0"
+protobuf = "2.16.2"
 
 [dev-dependencies]
 slog = "2.5"

diff --git a/vendor/mozilla-rust-sdk/googleapis-raw/src/empty.rs b/vendor/mozilla-rust-sdk/googleapis-raw/src/empty.rs
@@ -1,4 +1,4 @@
-// This file is generated by rust-protobuf 2.15.0. Do not edit
+// This file is generated by rust-protobuf 2.16.2. Do not edit
 // @generated
 
 // https://github.com/rust-lang/rust-clippy/issues/702
@@ -15,17 +15,13 @@
 #![allow(non_snake_case)]
 #![allow(non_upper_case_globals)]
 #![allow(trivial_casts)]
-#![allow(unsafe_code)]
 #![allow(unused_imports)]
 #![allow(unused_results)]
 //! Generated file from `empty.proto`
 
-use protobuf::Message as Message_imported_for_functions;
-use protobuf::ProtobufEnum as ProtobufEnum_imported_for_functions;
-
 /// Generated files are compatible only with the same version
 /// of protobuf runtime.
-// const _PROTOBUF_VERSION_CHECK: () = ::protobuf::VERSION_2_15_0;
+// const _PROTOBUF_VERSION_CHECK: () = ::protobuf::VERSION_2_16_2;
 
 #[derive(PartialEq,Clone,Default)]
 pub struct Empty {
@@ -95,7 +91,7 @@ impl ::protobuf::Message for Empty {
     fn as_any_mut(&mut self) -> &mut dyn (::std::any::Any) {
         self as &mut dyn (::std::any::Any)
     }
-    fn into_any(self: Box<Self>) -> ::std::boxed::Box<dyn (::std::any::Any)> {
+    fn into_any(self: ::std::boxed::Box<Self>) -> ::std::boxed::Box<dyn (::std::any::Any)> {
         self
     }
 
@@ -108,24 +104,20 @@ impl ::protobuf::Message for Empty {
     }
 
     fn descriptor_static() -> &'static ::protobuf::reflect::MessageDescriptor {
-        static mut descriptor: ::protobuf::lazy::Lazy<::protobuf::reflect::MessageDescriptor> = ::protobuf::lazy::Lazy::INIT;
-        unsafe {
-            descriptor.get(|| {
-                let fields = ::std::vec::Vec::new();
-                ::protobuf::reflect::MessageDescriptor::new_pb_name::<Empty>(
-                    "Empty",
-                    fields,
-                    file_descriptor_proto()
-                )
-            })
-        }
+        static descriptor: ::protobuf::rt::LazyV2<::protobuf::reflect::MessageDescriptor> = ::protobuf::rt::LazyV2::INIT;
+        descriptor.get(|| {
+            let fields = ::std::vec::Vec::new();
+            ::protobuf::reflect::MessageDescriptor::new_pb_name::<Empty>(
+                "Empty",
+                fields,
+                file_descriptor_proto()
+            )
+        })
     }
 
     fn default_instance() -> &'static Empty {
-        static mut instance: ::protobuf::lazy::Lazy<Empty> = ::protobuf::lazy::Lazy::INIT;
-        unsafe {
-            instance.get(Empty::new)
-        }
+        static instance: ::protobuf::rt::LazyV2<Empty> = ::protobuf::rt::LazyV2::INIT;
+        instance.get(Empty::new)
     }
 }
 
@@ -173,16 +165,14 @@ static file_descriptor_proto_data: &'static [u8] = b"\
     oto3\
 ";
 
-static mut file_descriptor_proto_lazy: ::protobuf::lazy::Lazy<::protobuf::descriptor::FileDescriptorProto> = ::protobuf::lazy::Lazy::INIT;
+static file_descriptor_proto_lazy: ::protobuf::rt::LazyV2<::protobuf::descriptor::FileDescriptorProto> = ::protobuf::rt::LazyV2::INIT;
 
 fn parse_descriptor_proto() -> ::protobuf::descriptor::FileDescriptorProto {
     ::protobuf::parse_from_bytes(file_descriptor_proto_data).unwrap()
 }
 
 pub fn file_descriptor_proto() -> &'static ::protobuf::descriptor::FileDescriptorProto {
-    unsafe {
-        file_descriptor_proto_lazy.get(|| {
-            parse_descriptor_proto()
-        })
-    }
+    file_descriptor_proto_lazy.get(|| {
+        parse_descriptor_proto()
+    })
 }