Remove query pool locks

[carver]

What was wrong?

The locks on the query pool are unnecessary. Locks suck. Remove them wherever possible.

How was it fixed?

Moving the query pool into a future::poll seems to be the only place that clone was required. It is relatively straightforward to route around that.

To-Do

• Clean up commit history and use conventional commits.

[KolbyML]

https://github.com/sigp/discv5/blame/682b51e317216b1e2ec3d75b541cdff66d27cf32/src/service.rs#L1547-L1562

All of this RFC/RFN code is originally based from sigp/discv5 implementation

Suggested change

	if let Poll::Ready(result) = Self::generate_query_event_poll(queries) {
	result
	} else {
	// sleep forever
	std::future::pending().await
	}
	}
	fn generate_query_event_poll<TQuery: Query<NodeId>>(
	queries: &mut QueryPool<NodeId, TQuery, TContentKey>,
	) -> Poll<QueryEvent<TQuery, TContentKey>> {
	match queries.poll() {
	future::poll_fn(move |_cx| match queries.poll() {

Can't we just do this?

Sigp doesn't use arc<mutex>'s either if you view the link I sent

[carver]

Probably? I'll keep working on it. This PR is definitely still a WIP. I just wanted to get it posted early because I remember Milos saying he might work on lock removal, and I want to avoid duplicate work.

[KolbyML]

I just brought it up because

            // sleep forever
            std::future::pending().await

^ Tastes weird,

and I think it can just be fully avoided by using future::poll_fn(move |_cx| match queries.poll() { instead (which is what sigp/discv5 does and they stopped using a lock here 4ish years ago)

[carver]

Oh I totally agree that that first implementation was not ideal. (it is worth checking, but my understanding is that the code is functionally equivalent, though)

The most important thing I wanted to do is see if the full test suite passes when the locks are gone. It looks like going back to the poll_fn implementation still works (can't remember what led me away from it at first), so the PR will almost certainly end up with that approach.

[KolbyML]

Sounds good 🚀

[carver]

@morph-dev you mentioned you were looking into other lock removal. I stumbled into this one, in the process of working on recursive find content. It feels unfinished: I probably want to refactor query_event_poll and its helper some more. But I figured I'd give an early heads up, in case you were starting to work on the same thing.

[carver]

For reference, the locks got added in #809

I don't remember the conversation at all, but apparently I didn't like them back then either 😆 : #809 (comment)

I took a peek at the PR, but didn't dive deep. I don't understand why they were added back then, really.

@njgheorghita any sense of why they were added, or if removing them could be a problem? Or does it all look good to you?

Also, you mentioned that you made an issue to remove the locks, but I didn't see it. Is there an issue somewhere that should close with this?

[KolbyML]

[morph-dev]

Thanks for the heads up. I was planning into looking into kbuckets, but my plan was not to remove the lock, but refactor them in a way so that they can't cause deadlock.

Regarding this change, I'm a bit worried that without lock, there could be some ugly race conditions. On one side, I believe that rust would prevent that, but on the other hand I'm a bit paranoid when it comes to multi-threading. It could also be due to my not so extensive familiarity with OverlayService (which is quite complex), so feel free to ignore my worry :).

[njgheorghita]

Looks good to me! Honestly, can't exactly remember the rationale for introducing the locks. I'm sure I had one, but it's not obvious that it was a good one since I looked back over that pr and couldn't figure it out. I'm guessing the "issue" I was referring to is just the pr description from #809, where you can see that I listed this as a follow-up and then never followed up 😅