chore: refactor node.rs into multiple modules #52
Conversation
src/node/connection.rs
Outdated
| } | ||
|
|
||
| impl NodeConn { | ||
| pub fn new(underlying: pallas_network::facades::NodeClient) -> Self { |
There was a problem hiding this comment.
In general I'm in favor of smaller files, but if you make NodeConn::new public to anyone, then we can no longer guarantee, that the module behaves soundly, e.g. calls .abort on line 105 of the new pool.rs.
Maybe let's make it pub(in crate::node), what do you think?
pub(in crate::node) fn new(underlying: pallas_network::facades::NodeClient) -> Self {
// …
}Cf. other options.
There was a problem hiding this comment.
Yes sure, not a bad idea and I don't have any requirements for its visibility.
Done in bdebfe1
Side note:
Maybe, in the future we can also improve dropping NodeClient by implementing the Drop trait somewhere. It will be triggered once the struct is freed from the memory.
There was a problem hiding this comment.
Thanks! 😌
I'd happily do that, but you can't run async code in a sync fn Drop::drop. At least not easily. You could create a new runtime there… Or message an existing runtime, which is what deadpool does, when their connection wrapper is dropped.
There's an AsyncDrop initiative [1] [2] in the async working group in the compiler team. This is also a list of more async issues for reference.
There was a problem hiding this comment.
@michalrus Yeah, I remember this from your Slack thread.
src/node/connection.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub async fn abort(&mut self) { |
There was a problem hiding this comment.
Now this is pub, and you allow passing a reference.
So after calling this, the user of NodeConn will end up with a NodeConn that has None as underlying. And after calling any other function, it will panic, since we use underlying.unwrap() everywhere else, because previously it was guaranteed that underlying will never be None.
I think the proper solution to this would be:
- Make
underlyingpub(in crate::node). - Remove
fn newandfn abortfromNodeConn. - Move their code to the pool.
This way you'll only have actual behavior methods like submit_transaction, ping, sync_progress in NodeConn, but it's complicated lifetime will be handled by the pool, and we can guarantee that it will be handled correctly.
There was a problem hiding this comment.
Yes, that sounds right, I agree with your proposal. However, I assumed these were further refactorings and didn't change the inner structure of the modules themselves. Do you mind if we refactor it with another PR? I need this restructuring to build upon my changes.
Even further, I would like to be able to split various methods like submit_transaction, ping, sync_progress to their own submodules/files but combine them in one module for usage. This is because each of these methods has the potential to hold complex logic alone.
There was a problem hiding this comment.
Okay, so we agreed in Slack that this would change the soundness of the interface. I pushed a fix in 9599a81.
Regarding splitting the interface into more files – definitely! Let's go for it.
|
We currently have both a node file and a node folder, which feels a bit confusing. 
|
|
|
nods sounds reasonable :) |
|
|
||
| /// Reports the sync progress of the node. | ||
| pub async fn sync_progress(&mut self) -> Result<SyncProgress, BlockfrostError> { | ||
| self.with_statequery(|generic_client: &mut localstate::GenericClient| { |
There was a problem hiding this comment.
This PR does:
NodeConn::newPlease note that namings or locations might be a little off. This is a first step towards trying to keep node stuff under control. (since it is probably going to get bigger)