feat(request): make request ID generation on client configurable #1535

PanGan21 · 2025-03-12T12:17:09Z

The following pr:

Adds Custom variant on IdKind
Adds an example that sets up a custom request id generator

Related: #1524

niklasad1

Hey hey,

Thanks for your PR and it looks generally good but I had in mind to add a the "custom generator" to the IdKind enum to avoid adding extra APIs for this. After this change one can configure the id_format with ClientBuilder::id_format and set a custom generator with ClientBuilder::with_request_id_generator. This means that with_request_id_generator overrides the configured id_format.

Thus, I would really like to unify them and perhaps replace these APIs with set_id_generator or something similar.

I reckon that we may need to refactor things and remove the next_id APIs with a similar generator impl as you added.

So, WDYT?

PanGan21 · 2025-03-12T15:57:45Z

Hey hey,

Thanks for your PR and it looks generally good but I had in mind to add a the "custom generator" to the IdKind enum to avoid adding extra APIs for this. After this change one can configure the id_format with ClientBuilder::id_format and set a custom generator with ClientBuilder::with_request_id_generator. This means that with_request_id_generator overrides the configured id_format.

Thus, I would really like to unify them and perhaps replace these APIs with set_id_generator or something similar.

I reckon that we may need to refactor things and remove the next_id AP with a similar generator impl as you added.

So, WDYT?

Thanks for the direction!
I will start the refactor as you described!
If necessary I might ask for some pointers since I am new to the repo.

niklasad1 · 2025-03-13T08:42:09Z

If necessary I might ask for some pointers since I am new to the repo.

All good, just ping us if you are blocked or need some extra help :)

core/src/client/mod.rs

client/http-client/src/client.rs

core/src/client/async_client/mod.rs

niklasad1 · 2025-03-18T14:10:47Z

client/http-client/src/client.rs

@@ -414,12 +414,11 @@ where
 			None => None,
 		};
 		let batch = batch.build()?;
-		let id = self.id_manager.next_request_id();
-		let id_range = generate_batch_id_range(id, batch.len() as u64)?;
+		let id_range = self.id_manager.generate_batch_id_range(batch.len());


Hey again,

So it doesn't make sense to have this id_range anymore because we are using it to check whether all
requests in a batch was replied to.

Now in the code the actual ID is fetched from the id_manager, thus it's not necessarily true that the id_range and self.id_manager.next_request_id() contains the same ids.

Additionally, because it's possible to use a custom id generator we can't really on these ID ranges anymore anyway.

I reckon that we need to remove the generate_batch_id_range completely and just push the generated IDs to a Vec such as:

// this will contain the order of the request IDs in the batch let mut ids = Vec::new(); for (method, params) in batch.into_iter() { let id = self.id_manager.next_request_id(); batch_request.push(RequestSer { jsonrpc: TwoPointZero, id, method: method.into(), params: params.map(StdCow::Owned), }); } // use ids to check whether to response has responded to all ids in the batch

The reason why we need this is because a server can submit respond to each call in the batch in arbitrary order.

The Response objects being returned from a batch call MAY be returned in any order within the Array. The Client SHOULD match contexts between the set of Request objects and the resulting set of Response objects based on the id member within each Object.

The actual logic to match a batch response to the batch request must be also be changed because one assumption is that the every ID is generated from a integer and can be parsed as one which is used to index into the response.

One extra test with an unordered batch response would be great to make sure everything works as intended.

Thanks for the feedback - makes sense.
According to that, the usage of Range type in BatchMessage and in related functions, should also be changed right? It should probably be replaces with something like Vec<Id<'static>>.

yes, correct

FYI the refactor is in progress but I am facing some issues with the lifetimes. I need to either use Vec<Id<'a>> which means that a lifetime parameter will be added across a big part of the client or use Vec<Id<'static>> which is always not optimal.

just do the naive implementation now with Vec<Id<'static>> the default one is still number and cloning it should be "cheap".

Updated and test added

PanGan21 · 2025-03-20T20:57:13Z

core/src/client/async_client/manager.rs

+		for (key, _) in self.batches.iter() {
+			if key.len() == batch.len() && batch.iter().all(|id| key.contains(id)) {
+				matched_key = Some(key.clone());
+				break;
 			}
-			_ => None,
 		}
+
+		if let Some(key) = matched_key {
+			if let Some((_key, state)) = self.batches.remove_entry(&key) {
+				return Some(state);
+			}
+		}
+
+		None


Most likely this could improved if a different data structure is used to store batches on RequestManager

niklasad1 · 2025-03-21T13:35:45Z

client/http-client/src/tests.rs

+	batch_request.insert("test_echo", rpc_params!["fourth"]).unwrap();
+	batch_request.insert("test_echo", rpc_params!["fifth"]).unwrap();
+	let server_response = r#"[{"jsonrpc":"2.0","result":"fifth","id":4}, {"jsonrpc":"2.0","result":"hello","id":0}, {"jsonrpc":"2.0","result":"here's your swag","id":2}, {"jsonrpc":"2.0","result":"fourth","id":3}, {"jsonrpc":"2.0","result":"goodbye","id":1}]"#.to_string();
+	let res = run_batch_request_with_response::<String>(batch_request, server_response)


can you please change this to use a custom id i.e., you need to set it by Client::id_format? 🙏

niklasad1 · 2025-03-21T14:35:14Z

cool cool, I'll have a proper look next week thanks for working on this

niklasad1 · 2025-04-01T15:54:29Z

Sorry @PanGan21 I'll take a look tomorrow 🙏

niklasad1 · 2025-04-14T13:26:43Z

core/src/client/async_client/helpers.rs

-		let err_obj = ErrorObject::borrowed(0, "", None);
-		responses.push(Err(err_obj));
-	}
+	let mut responses_map: BTreeMap<Id<'static>, Result<_, ErrorObject>> =


We can't use a BTreeMap here because there is nothing that prevents an ID (duplicate ids) to be used more than once with a custom ID generator

niklasad1 · 2025-04-14T13:33:13Z

core/src/client/async_client/manager.rs

+	pub(crate) fn complete_pending_batch(&mut self, batch: Vec<Id<'static>>) -> Option<BatchState> {
+		let mut matched_key = None;
+
+		for (key, _) in self.batches.iter() {


yeah, this is inefficient/annoying compared to the old code 🤔

niklasad1 · 2025-04-14T13:56:47Z

core/src/client/async_client/manager.rs

+		let mut matched_key = None;
+
+		for (key, _) in self.batches.iter() {
+			if key.len() == batch.len() && batch.iter().all(|id| key.contains(id)) {


you could just use Vec partialeq/eq impl here.

niklasad1 · 2025-04-14T14:00:16Z

core/src/client/async_client/manager.rs

 		}
+
+		if let Some(key) = matched_key {
+			if let Some((_key, state)) = self.batches.remove_entry(&key) {


It doesn't make sense to use a hashmap here anymore, it should most likely be Vec<Vec<Id<'static'>> or something. Then just use do Vec::retain to remove the completed batch

Fixed. Not using retain but a similar approach. Let me know if that works for you

niklasad1

Thanks for your PR (sorry about slow review though)

I'm a bit annoyed how inefficient the implementation becomes with custom id generator, by using Vec<Id> all over the place and to search for element in the batch response.

It's not possible to use BTreeMap/HashMap because we can't assume that duplicated IDs are not used by custom request ID generator.

So, what we could do is to sort Vec<Id> and then binary search through it

However, I'm not convinced that this annoying implementation is worth the effort for such a niche feature though

//cc @jsdw what do you think? Or any smarter ideas?

PanGan21 · 2025-04-15T13:22:40Z

For some reason after rebase with master branch, the test batch_request_out_of_order_response succeeds on http but fails on ws. Trying to understand why but in case there is an obvious reason and I don't see it please let me know

PanGan21 requested a review from a team as a code owner March 12, 2025 12:17

PanGan21 mentioned this pull request Mar 12, 2025

feature request: make request ID generation on client configurable #1524

Open

niklasad1 reviewed Mar 12, 2025

View reviewed changes

implement custom generator on IdKind

51ad234

PanGan21 force-pushed the 1524-request-id-generator branch from 481c077 to 51ad234 Compare March 13, 2025 10:27

PanGan21 commented Mar 13, 2025

View reviewed changes

core/src/client/mod.rs Outdated Show resolved Hide resolved

refactor next request id to handle the next id

0ee2516

PanGan21 commented Mar 14, 2025

View reviewed changes

core/src/client/mod.rs Show resolved Hide resolved

niklasad1 reviewed Mar 14, 2025

View reviewed changes

client/http-client/src/client.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Mar 14, 2025

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

PanGan21 added 2 commits March 15, 2025 21:09

move batch id generation to request id manager

8f7b381

simplify implementation

150f07d

PanGan21 requested a review from niklasad1 March 17, 2025 15:13

niklasad1 reviewed Mar 18, 2025

View reviewed changes

PanGan21 and others added 2 commits March 20, 2025 21:54

refactor: replaces ranges with vectors of ids

3d714a0

Merge branch 'master' into 1524-request-id-generator

b57905d

PanGan21 commented Mar 20, 2025

View reviewed changes

PanGan21 requested a review from niklasad1 March 20, 2025 20:57

add test with an unordered batch response

556ee8a

niklasad1 reviewed Mar 21, 2025

View reviewed changes

change test to use custom id generator

e4005ac

niklasad1 reviewed Apr 14, 2025

View reviewed changes

PanGan21 and others added 4 commits April 14, 2025 23:05

fix conflicts

a8c2689

fix example

ebae283

refactor batches from hashmap to vec

607226a

Merge branch 'master' into 1524-request-id-generator

4f6b8e9

niklasad1 mentioned this pull request Apr 21, 2025

client: rpc middleware could modify the request ID (footgun) #1565

Open

feat(request): make request ID generation on client configurable #1535

Are you sure you want to change the base?

feat(request): make request ID generation on client configurable #1535

Uh oh!

Conversation

PanGan21 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niklasad1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PanGan21 commented Mar 12, 2025

Uh oh!

niklasad1 commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Mar 21, 2025

Uh oh!

niklasad1 commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

PanGan21 commented Apr 15, 2025

Uh oh!

Uh oh!

PanGan21 commented Mar 12, 2025 •

edited

Loading

niklasad1 left a comment •

edited

Loading

niklasad1 Mar 18, 2025 •

edited

Loading