Skip to content

Commit f53b960

Browse files
oxadeoxade
authored
Disallow futures::executor::block_on (MystenLabs#10912)
## Description * Adds clippy warning wherever `futures::executor::block_on` is used * Area owners should address. Looks like mostly in indexer, narwhal, sui-json, sui-json-rpc, sui-core For more context see: MystenLabs#10910 ## Test Plan N/A --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
1 parent bd2b509 commit f53b960

File tree

6 files changed

+113
-27
lines changed

6 files changed

+113
-27
lines changed

‎.clippy.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ disallowed-methods = [
99
{ path = "tokio::sync::mpsc::unbounded_channel", reason = "use a bounded channel instead" },
1010
{ path = "futures::channel::mpsc::unbounded", reason = "use a bounded channel instead" },
1111
{ path = "futures_channel::mpsc::unbounded", reason = "use a bounded channel instead" },
12+
# known to cause blocking issues
13+
{ path = "futures::executor::block_on", reason = "use tokio::runtime::runtime::Runtime::block_on instead"},
1214
# bincode::deserialize_from is easy to shoot your foot with
1315
{ path = "bincode::deserialize_from", reason = "use bincode::deserialize instead" },
1416
]

‎crates/sui-indexer/benches/indexer_benchmark.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ fn indexer_benchmark(c: &mut Criterion) {
3737
let pw = env::var("POSTGRES_PASSWORD").unwrap_or_else(|_| "postgrespw".into());
3838
let db_url = format!("postgres://postgres:{pw}@{pg_host}:{pg_port}");
3939

40-
let rt = Runtime::new().unwrap();
40+
let rt: Runtime = Runtime::new().unwrap();
4141
let (mut checkpoints, store) = rt.block_on(async {
4242
let (blocking_cp, async_cp) = new_pg_connection_pool(&db_url).await.unwrap();
4343
reset_database(&mut blocking_cp.get().unwrap(), true).unwrap();

‎crates/sui-indexer/src/apis/indexer_api.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,8 @@ impl<S> IndexerApiServer for IndexerApi<S>
318318
where
319319
S: IndexerStore + Sync + Send + 'static,
320320
{
321+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
322+
#[allow(clippy::disallowed_methods)]
321323
fn get_owned_objects(
322324
&self,
323325
address: SuiAddress,
@@ -343,7 +345,8 @@ where
343345
}
344346
block_on(self.get_owned_objects_internal(address, query, cursor, limit))
345347
}
346-
348+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
349+
#[allow(clippy::disallowed_methods)]
347350
fn query_transaction_blocks(
348351
&self,
349352
query: SuiTransactionBlockResponseQuery,
@@ -377,6 +380,8 @@ where
377380
))?)
378381
}
379382

383+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
384+
#[allow(clippy::disallowed_methods)]
380385
fn query_events(
381386
&self,
382387
query: EventFilter,
@@ -407,6 +412,8 @@ where
407412
))?)
408413
}
409414

415+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
416+
#[allow(clippy::disallowed_methods)]
410417
fn get_dynamic_fields(
411418
&self,
412419
parent_object_id: ObjectID,
@@ -426,6 +433,8 @@ where
426433
df_resp
427434
}
428435

436+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
437+
#[allow(clippy::disallowed_methods)]
429438
fn get_dynamic_field_object(
430439
&self,
431440
parent_object_id: ObjectID,

‎crates/sui-indexer/src/apis/read_api.rs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ impl<S> ReadApiServer for ReadApi<S>
128128
where
129129
S: IndexerStore + Sync + Send + 'static,
130130
{
131+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
132+
#[allow(clippy::disallowed_methods)]
131133
fn get_object(
132134
&self,
133135
object_id: ObjectID,
@@ -147,6 +149,8 @@ where
147149
Ok(block_on(self.get_object_internal(object_id, options))?)
148150
}
149151

152+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
153+
#[allow(clippy::disallowed_methods)]
150154
fn multi_get_objects(
151155
&self,
152156
object_ids: Vec<ObjectID>,
@@ -203,6 +207,8 @@ where
203207
.await?)
204208
}
205209

210+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
211+
#[allow(clippy::disallowed_methods)]
206212
fn multi_get_transaction_blocks(
207213
&self,
208214
digests: Vec<TransactionDigest>,
@@ -227,6 +233,8 @@ where
227233
)?)
228234
}
229235

236+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
237+
#[allow(clippy::disallowed_methods)]
230238
fn try_get_past_object(
231239
&self,
232240
object_id: ObjectID,
@@ -246,6 +254,8 @@ where
246254
past_obj_resp
247255
}
248256

257+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
258+
#[allow(clippy::disallowed_methods)]
249259
fn try_multi_get_past_objects(
250260
&self,
251261
past_objects: Vec<SuiGetPastObjectRequest>,
@@ -301,6 +311,8 @@ where
301311
Ok(self.state.get_checkpoint(id).await?)
302312
}
303313

314+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
315+
#[allow(clippy::disallowed_methods)]
304316
fn get_checkpoints(
305317
&self,
306318
cursor: Option<BigInt<u64>>,
@@ -320,6 +332,8 @@ where
320332
cps_resp
321333
}
322334

335+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
336+
#[allow(clippy::disallowed_methods)]
323337
fn get_checkpoints_deprecated_limit(
324338
&self,
325339
cursor: Option<BigInt<u64>>,
@@ -329,6 +343,8 @@ where
329343
self.get_checkpoints(cursor, limit.map(|l| *l as usize), descending_order)
330344
}
331345

346+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
347+
#[allow(clippy::disallowed_methods)]
332348
fn get_events(&self, transaction_digest: TransactionDigest) -> RpcResult<Vec<SuiEvent>> {
333349
let events_guard = self
334350
.state
@@ -339,7 +355,8 @@ where
339355
events_guard.stop_and_record();
340356
events_resp
341357
}
342-
358+
// TODO: remove this after `futures::executor::block_on` is removed. @Ge @Chris
359+
#[allow(clippy::disallowed_methods)]
343360
fn get_loaded_child_objects(
344361
&self,
345362
digest: TransactionDigest,

‎crates/sui-tool/src/replay.rs

Lines changed: 80 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,41 @@ pub enum LocalExecError {
201201

202202
#[error("GeneralError: {:#?}", err)]
203203
GeneralError { err: String },
204+
205+
#[error("ObjectNotExist: {:#?}", id)]
206+
ObjectNotExist { id: ObjectID },
207+
208+
#[error(
209+
"ObjectDeleted: {:#?} at version {:#?} digest {:#?}",
210+
id,
211+
version,
212+
digest
213+
)]
214+
ObjectDeleted {
215+
id: ObjectID,
216+
version: SequenceNumber,
217+
digest: ObjectDigest,
218+
},
219+
}
220+
221+
impl From<SuiObjectResponseError> for LocalExecError {
222+
fn from(err: SuiObjectResponseError) -> Self {
223+
match err {
224+
SuiObjectResponseError::NotExists { object_id } => {
225+
LocalExecError::ObjectNotExist { id: object_id }
226+
}
227+
SuiObjectResponseError::Deleted {
228+
object_id,
229+
digest,
230+
version,
231+
} => LocalExecError::ObjectDeleted {
232+
id: object_id,
233+
version,
234+
digest,
235+
},
236+
_ => LocalExecError::SuiObjectResponseError { err },
237+
}
238+
}
204239
}
205240

206241
impl From<LocalExecError> for SuiError {
@@ -389,6 +424,8 @@ impl LocalExec {
389424
Ok(objects)
390425
}
391426

427+
// TODO: remove this after `futures::executor::block_on` is removed.
428+
#[allow(clippy::disallowed_methods)]
392429
pub fn download_object(
393430
&self,
394431
object_id: &ObjectID,
@@ -436,24 +473,42 @@ impl LocalExec {
436473
Ok(o)
437474
}
438475

439-
pub fn download_latest_object(&self, object_id: &ObjectID) -> Result<Object, LocalExecError> {
440-
// TODO: replace use of `block_on`
441-
block_on(self.download_latest_object_impl(object_id))
476+
// TODO: remove this after `futures::executor::block_on` is removed.
477+
#[allow(clippy::disallowed_methods)]
478+
pub fn download_latest_object(
479+
&self,
480+
object_id: &ObjectID,
481+
) -> Result<Option<Object>, LocalExecError> {
482+
block_on({
483+
info!("Downloading latest object {object_id}");
484+
self.download_latest_object_impl(object_id)
485+
})
442486
}
443487

444488
pub async fn download_latest_object_impl(
445489
&self,
446490
object_id: &ObjectID,
447-
) -> Result<Object, LocalExecError> {
491+
) -> Result<Option<Object>, LocalExecError> {
448492
let options = SuiObjectDataOptions::bcs_lossless();
449-
let object = self
450-
.client
451-
.read_api()
452-
.get_object_with_options(*object_id, options)
453-
.await
454-
.map_err(|q| LocalExecError::SuiRpcError { err: q.to_string() })?;
455493

456-
obj_from_sui_obj_response(&object)
494+
self
495+
.client
496+
.read_api()
497+
.get_object_with_options(*object_id, options)
498+
.await.map(|q| match obj_from_sui_obj_response(&q){
499+
Ok(v) => Ok(Some(v)),
500+
Err(LocalExecError::ObjectNotExist { id }) => {
501+
error!("Could not find object {id} on RPC server. It might have been pruned, deleted, or never existed.");
502+
Ok(None)
503+
}
504+
Err(LocalExecError::ObjectDeleted { id, version, digest }) => {
505+
error!("Object {id} {version} {digest} was deleted on RPC server.");
506+
Ok(None)
507+
},
508+
Err(err) => Err(LocalExecError::SuiRpcError {
509+
err: err.to_string(),
510+
})
511+
})?
457512
}
458513

459514
pub async fn execute_all_in_checkpoint(
@@ -624,13 +679,16 @@ impl LocalExec {
624679
Ok(())
625680
}
626681

627-
pub fn get_or_download_object(&self, obj_id: &ObjectID) -> Result<Object, LocalExecError> {
682+
pub fn get_or_download_object(
683+
&self,
684+
obj_id: &ObjectID,
685+
) -> Result<Option<Object>, LocalExecError> {
628686
if let Some(obj) = self.package_cache.lock().expect("Cannot lock").get(obj_id) {
629-
return Ok(obj.clone());
687+
return Ok(Some(obj.clone()));
630688
};
631689

632690
let o = match self.store.get(obj_id) {
633-
Some(obj) => obj.clone(),
691+
Some(obj) => Some(obj.clone()),
634692
None => {
635693
assert!(
636694
!self.system_package_ids().contains(obj_id),
@@ -639,6 +697,7 @@ impl LocalExec {
639697
self.download_latest_object(obj_id)?
640698
}
641699
};
700+
let Some(o) = o else { return Ok(None) };
642701

643702
if o.is_package() {
644703
self.package_cache
@@ -651,7 +710,7 @@ impl LocalExec {
651710
.lock()
652711
.expect("Cannot lock")
653712
.insert((o_ref.0, o_ref.1), o.clone());
654-
Ok(o)
713+
Ok(Some(o))
655714
}
656715

657716
/// Must be called after `populate_protocol_version_tables`
@@ -1061,8 +1120,7 @@ impl BackingPackageStore for LocalExec {
10611120
fn get_package_object(&self, package_id: &ObjectID) -> SuiResult<Option<Object>> {
10621121
// If package not present fetch it from the network
10631122
self.get_or_download_object(package_id)
1064-
.map(Some)
1065-
.map_err(|e| e.into())
1123+
.map_err(|e| SuiError::GenericStorageError(e.to_string()))
10661124
}
10671125
}
10681126

@@ -1101,7 +1159,9 @@ impl ResourceResolver for LocalExec {
11011159
address: &AccountAddress,
11021160
typ: &StructTag,
11031161
) -> Result<Option<Vec<u8>>, Self::Error> {
1104-
let object: Object = self.get_or_download_object(&ObjectID::from(*address))?;
1162+
let Some(object) = self.get_or_download_object(&ObjectID::from(*address))? else {
1163+
return Ok(None);
1164+
};
11051165

11061166
match &object.data {
11071167
Data::Move(m) => {
@@ -1189,12 +1249,8 @@ impl GetModule for LocalExec {
11891249
}
11901250

11911251
fn obj_from_sui_obj_response(o: &SuiObjectResponse) -> Result<Object, LocalExecError> {
1192-
let o: Result<SuiObjectData, anyhow::Error> = Ok(o
1193-
.object()
1194-
.map_err(|q| LocalExecError::SuiObjectResponseError { err: q })?
1195-
.clone());
1196-
1197-
obj_from_sui_obj_data(&o.map_err(|q| LocalExecError::GeneralError { err: q.to_string() })?)
1252+
let o = o.object().map_err(LocalExecError::from)?.clone();
1253+
obj_from_sui_obj_data(&o)
11981254
}
11991255

12001256
fn obj_from_sui_obj_data(o: &SuiObjectData) -> Result<Object, LocalExecError> {

‎narwhal/.clippy.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ disallowed-methods = [
1212
{ path = "log::debug", reason = "use tracing::debug instead" },
1313
{ path = "log::error", reason = "use tracing::error instead" },
1414
{ path = "log::warn", reason = "use tracing::warn instead" },
15+
# known to cause blocking issues
16+
{ path = "futures::executor::block_on", reason = "use tokio::runtime::runtime::Runtime::block_on instead"},
1517
# unbounded channels are for expert use only
1618
{ path = "tokio::sync::mpsc::unbounded_channel", reason = "use a bounded channel instead" },
1719
{ path = "futures::channel::mpsc::unbounded", reason = "use a bounded channel instead" },

0 commit comments

Comments
 (0)