Streamlining batch_call and removing unnecessary unwraps() #34
Conversation
dr-orlovsky
force-pushed
the
fix/batch_call
branch
2 times, most recently
from
b943c56
to
02462b3
Compare
src/raw_client.rs
Outdated
| let mut guard = self | ||
| .waiting_map | ||
| .lock() | ||
| .map_err(|_| Error::CouldntLockReader)?; |
There was a problem hiding this comment.
Same as in #33, this needs a specific error
src/raw_client.rs
Outdated
| .waiting_map | ||
| .lock() | ||
| .map_err(|_| Error::CouldntLockReader)?; | ||
| guard.remove(&req_id); |
There was a problem hiding this comment.
I think I'm missing something, why did you remove the loop here?
There was a problem hiding this comment.
Yep,my bad, I haven't understood the code and thought req_id is an external variable. Will fix. Now I see why you was maintaining list of missing responses up to date.
In general, can you explain what actually should happen here? What should be a consequences of erroring one of the batch request? Cancellation of the rest of the requests and returning single error, ignoring all ok-requests already received? That does not make a lot of sense for me... May be I am missing something?
There was a problem hiding this comment.
Right now if one request fails it makes the whole batch fail. If there are still some requests pending it will just ignore their response when they come back (there's no way of cancelling a request you've already sent).
This line specifically is where all the pending requests id are removed from the map of responses we are waiting for, which means that once the response comes in the current reader_thread will just ignore it:
rust-electrum-client/src/raw_client.rs
Lines 493 to 500 in 02462b3
src/raw_client.rs
Outdated
| Ok(resp) => { | ||
| let mut res = resp | ||
| .get("result") | ||
| .cloned() |
There was a problem hiding this comment.
I would keep this part as it was before, just take()-ing the result and adding it to answer.
There's no need to return an InvalidResponse here because if there's something wrong with it we'll just see that later when trying to parse the response into a specific struct.
I guess if you really wanted to return that error instead of a generic serde error you could add this to impl_batch_call:
rust-electrum-client/src/raw_client.rs
Line 49 in af10aea
You could map the error here to InvalidResponse before returning it
src/raw_client.rs
Outdated
| let answer = answer.into_iter().map(|mut x| x["result"].take()).collect(); | ||
|
|
||
| Ok(answer) | ||
| Ok(answers.into_iter().map(|(_, item)| item).collect()) |
There was a problem hiding this comment.
I think you are missing the part where you remove the id of the response we've just received from missing_responses.
dr-orlovsky
force-pushed
the
fix/batch_call
branch
from
02462b3
to
d64aaff
Compare
|
Fixed according to suggestions & improved |
src/raw_client.rs
Outdated
| for req_id in missing_responses.iter() { | ||
| match self.recv(&receiver, *req_id) { | ||
| Ok(mut resp) => answers.insert( | ||
| resp["id"].as_u64().unwrap_or_default(), |
There was a problem hiding this comment.
I'm not entirely sure unwrap_or_default() is a good idea here: in theory if we got to this point it means that the response had an id, but in case it's missing assuming its value is 0 can create issues. I think it would be better to replace it with .unwrap_or(req_id), or even just always returning the req_id directly.
There was a problem hiding this comment.
You are right. Fixed in cfc7bfe
This improves overall function code style & allows to remove two unnecessary unwraps and one implicit panic. Together with #33 this makes this library unwrap-free and stable (i.e. not causing the software using it to unexpectedly terminate instead or reporting error to the calling function)