Refactor tcp #749
Refactor tcp #749
Conversation
|
Ah, failed to check go.mod. Is it safe to try and upgrade to go 1.21? Or I can just do the check differently ( edit: updating go.mod would probably be a separate PR, will change it for go 1.20 |
This refactor uses the functional options pattern to extract away the optional TCP server configuration parameters. This: - improves overall readability, by moving away from global variables - makes it easier to configure the tcp server for tests
Go offers a ticker abstraction designed for performing tasks at a regular interval, and this change uses the ticker for tcp room deletion. It also cleans up the deletion goroutine gracefully.
The diagram sketches out the interaction between clients and a local relay.
|
looks good, do you feel like its ready to merge? |
|
Should be ready, the only functional change is room removal (using different constructs, it's supposed to work the same). But I have some free time tomorrow, can test the new version in more network conditions than just local, just to be extra sure nothing breaks. I'll let you know once I've done that UPDATE: not convinced the stopping mechanism for the room deletion goroutine is actually doing something useful, in which case maybe I wouldn't add it in this PR to keep the changeset to a minimum (i.e. I believe it doesn't do anything because it looks like the TCP server is not closed once the transfer is done). Otherwise, seem to work as expected to me. |
These would be useful for future development (e.g. adding a stopping mechanism for the TCP listener).
|
Given the room stopping isn't actually called, I think this change should be functionally equivalent to the previous code and is ready to be merged. I'd still keep the deletion mechanism after all, since I believe it would work for shutting down gracefully that goroutine, when that would be implemented. I also added some debug logs for the cleanup. |
I'm still working on #743, but while trying to figure out if the change works in all cases (still not 100% convinced I fully understand what happens when the sender starts a local relay, and haven't yet considered peer discovery), I saw some potential stylistic improvements for the TCP implementation. This PR does not change any functionality, and is mostly focused around the configuration of the TCP listener. Each individual commit will describe the change in more detail.
I also added a diagram for what I think happens when connecting to the local relay. I drew it for myself initially, but thought it might help other contributors.
Let me know if you like the refactor and want me to extend it further (e.g. refactor the
Runfunction completely, or tests) The way I wrote the refactor was meant to change the least amount of files possible.Tests pass on linux. On windows they fail on the main branch too, probably because the networking is implemented differently.