search

L2-Technology / sensei

A lightning node implementation for everyone
https://l2.technology/sensei
Other
198 stars 39 forks source link

Get rid of anti-pattern Some(Some(_)) #83

Closed benthecarman closed 2 years ago

johncantrell97 commented 2 years ago

Yeah, this is ugly as all hell. The problem is this PR is just a bandaid hiding the real problem. The node_directory needs to be re-worked slightly to not need this double Option in the first place.

Basically the idea is that the directory is supposed to be a map from Pubkey => Running Node Handle.

Which would normally just be HashMap<String, NodeHandle> but there's another issue where you don't want concurrent start requests to start the same node more than once. So you need a Mutex to lock the startup of a node until it can be added to the directory.

As a hack I just re-used the same directory and made it an Option in the map so that I could put a None value into the map when the node was starting and replace it with Some when it was started.

But this means when you check if a Pubkey exists in the map you have this weird nested state of if it's not in there then it means it's not running AND no one is starting it so it's safe to start it.

If it exists in the map and is None then it means it's being started but hasn't started yet and you should basically do nothing (or error back, already starting)

If it exists in the map and is Some(Handle) then it's already started and you're good to go.

So I guess the actual fix here is to just use a separate mutex for pubkey's that are currently being started. This way if something is in the directory then you know it's running and can avoid this awkward double option situation.

thoughts?

benthecarman commented 2 years ago

I think the best way to handle it would be inside the start function where there it accesses the mutex to see if the node is currently starting up or not. This way you wouldn't need to remember to check the mutex before calling start because it is handled within the function. However I am very new to this codebase

johncantrell97 commented 2 years ago

Yeah, definitely inside start. I guess I meant I think the best solution is to introduce a second Mutex that is only used to track starting nodes -- something like Mutex<HashMap<String, ()>>. This way the node_directory can go back to just being a map for running nodes and won't need to stuff another Option in that node_directory map.

benthecarman commented 2 years ago

Closing as this is just an band-aid fix like @johncantrell97 explained