Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose torrent Peer status updates #987

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marcovidonis
Copy link
Collaborator

Provide updates on peer status which include peer ID, ok status, and an error message. This can be useful for the client to be notified of errors when running a connection after handshake.

Updates are sent via channels to Observers, which are set up by the client in config.

marcovidonis and others added 4 commits October 15, 2024 11:52
* basic observer framework
* connects to a valid tracker
* added observer channel for announce status
* set up Peer Connection status Observers
* add PeerConn test: connection established
* added Observers factory method
* Added Event to AnnounceStatus, with embedded TrackerStatus
* state updates must be non-blocking
* add unit tests on PeerConn Observer status reading
* add test and debug log on dropped connection
* add PeerID check to test

---------

Co-authored-by: Parker Whittle <[email protected]>
@anacrolix
Copy link
Owner

I'm not sure about this one. I don't think channels are a good way to handle events unless there's explicit concurrency cooperation required. In the case here where we are updating status information, there's no cooperation, we're just telling users of events. I think I started setting up https://pkg.go.dev/github.com/anacrolix/torrent#Callbacks as a way to handle various events that occur in the client. In particular if you wanted to have a subset of this exposed to various subpackages, like trackers, webtorrent etc. you would create a callback struct for each of those and embed it in the larger one in the client. Having synchronous events lets the user act on or even alter behaviour as appropriate. As for the various status objects, what do you think of just exposing those internal types where possible instead? I believe I started this process for Peer, PeerConn, Torrent and a few others, and their public API is mixed but could be extended.

In general I think introducing observers, and meta types to track their state creates new API contracts that are particularly hard to navigate around. I could be completely wrong but it just doesn't feel right. There have been plenty of bad API choices in anacrolix/torrent to date that I'm stuck with.

@marcovidonis
Copy link
Collaborator Author

Thanks for getting back to me on this. I generally like channels as I feel I can give real-time updates as things happen, but I see your point and I'll look into changing this into callbacks (when I get some time).
As for exposing internal types, I think I picked up the pattern of writing getter functions early on, and only recently learned that it's actually not a recommended pattern! So yes, I totally agree with just exposing those types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants