server: Integrate reconnect tasks into ChiaServer

[xdustinface]

Just feels like it belongs in there rather than in its own file because we pass in an ChiaServer anyway and access its connections and starting the client.

Based on:

• server: Resolve config peers earlier #14069
• simulator: Replace localhost -> 127.0.0.1 in farmer/harvester config #14165

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[arvidn]

this is not an obvious improvement to me. for example, if we ever were to write unit tests for this function, having it be a free function would make that a lot simpler (we would just need to come up with a protocol to mock the ChiaServer we pass in, or pass in the two members we access separately).

There are a few other changes in here that seems like obvious improvements though (and also somewhat unrelated to moving the free function into the class). for example, moving the exception type into the error file

[xdustinface]

this is not an obvious improvement to me. for example, if we ever were to write unit tests for this function, having it be a free function would make that a lot simpler (we would just need to come up with a protocol to mock the ChiaServer we pass in, or pass in the two members we access separately).

Actually it seemed clear to me that it's part of ChiaServer. Since it's not obvious to you, can you explain where you draw the line of making a function accessing internals of a class a method vs. free function?

Not sure why a maybe coming unit test would justify leaving this method in its own file with a public interface vs integrating it as private method into the related class? Also i don't really get how the "pass in the two members separately" would look like with this being a task which access them at runtime (not only on task creation) other than holding a reference to the passed in members in the task which seems risky/unintuitive?

There are a few other changes in here that seems like obvious improvements though (and also somewhat unrelated to moving the free function into the class). for example, moving the exception type into the error file

There are few somewhat unrelated simplifications, right, like getting rid of the inner wrapping method and instead create the task in place or simplify the "are we still connected check". But moving the exception isn't unrelated imo. It's just, there is a ServiceException before and now there is a ServerException because the PR integrated the reconnecting from the service into the server moving the exception is just to not have an exception in the wrong place.

[arvidn]

is start_service.py the only user of ChiaServer?
By moving this logic from start_service into ChiaServer you seem to suggest that is the case. If not, this is a behavior change

[arvidn]

accessing the internal member her will make it harder to change implementation details of harvester_service, this increases technical debt. I would prefer tests use the public interface.

[arvidn]

do you know why we have a reconnect task for each peer? It seems really wasteful for every peer to iterate over all peers just to find itself. It would seem simpler an more efficient to just have one task that iterates over all peers and retries the ones that should be retried

[arvidn]

nothing this function accesses from server starts with an underscore. I would think all_connections and start_client() are meant to be public

[arvidn]

Actually it seemed clear to me that it's part of ChiaServer. Since it's not obvious to you, can you explain where you draw the line of making a function accessing internals of a class a method vs. free function?

I think the short answer is: where it makes sense to chop up logic to be unit tested.

Not sure why a maybe coming unit test would justify leaving this method in its own file with a public interface vs integrating it as private method into the related class?

I would say it's increasing the coupling between those two parts. Generally I think we should be moving in the direction of splitting classes into smaller pieces. That makes them much easier to unit test.

Also i don't really get how the "pass in the two members separately" would look like with this being a task which access them at runtime (not only on task creation) other than holding a reference to the passed in members in the task which seems risky/unintuitive?

For example, the peer list could be passed in separately from the function to reconnect a peer. This way it would make a test much simpler because you wouldn't have to create something that looks like a ChiaServer sharing references to mutable data isn't very nice, but it's how it works now. But perhaps more importantly, it doesn't look like a very good idea to have a task per peer for this to begin with.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[xdustinface]

Closing in favour of #14997.