middleware/hooks: preserve prehook response changes

[shyba]

Hi, thanks for this amazing project.

While implementing a prehook middleware, my changes to IPv4Peers were being discarded. This is what I had to do in order to fix it.

(I don't know if it is expected for prehook middleware implementations to modify responses, so feel free to discard the PR as well. The middleware I am working on needs to inject peers and using this hook was the cleanest approach.)

[jzelinskie]

Hey there!

Yeah, this behavior is definitely up for discussion.
I think it makes sense for the semantics to be append and not replacing all peers from middleware before it.

What say you @mrd0ll4r?

[mrd0ll4r]

Hmm. This kind of disrespects numWant now, because the storage will return numWant peers, which we potentially append to some existing peers. This also has subtle interactions with Complete and Incomplete, I think. Posthooks have those as well, but they can modify the response accordingly, if they want.

What's the reasoning on your end to implement this as a prehook instead of a posthook, @shyba ?

EDIT: Thinking about it some more, the most elegant approach would be to pass a (possibly non-empty) slice of bittorrent.Peer to the storage to fill up to numWant, but we can't do that with the current interfaces. So imho the preferred way to modify responses is via posthooks.

[shyba]

Hi, sorry for the delay. I'm traveling for some days. I tried using it as a posthook, but it looks like the posthook runs after the response is sent on a thread? At least that is what I have on history from last I tried. I will try again as soon as I'm back. This is the code I saw that looks like running after the response is sent: https://github.com/chihaya/chihaya/blob/7455c2ad4a5860d7f73b98cae4e4160e194fb45b/frontend/udp/frontend.go#L322 This is my middleware implementation: https://github.com/shyba/chihaya/tree/main/middleware/fixedpeer (The other changes on that branch are work in progress, but I plan on upstreaming everything that makes sense when it is done. The one on ports is related to "anonymous mode" where some torrent clients will send 0 or 1 as invalid port to query without announcing)

[mrd0ll4r]

(replying via email via phone, also traveling, hope this works)Oh, right, that's how that worked.I guess this is fine then. Please also open an issue to change the storage interface to take a slice of peers as an input, for the future 😊On Apr 17, 2022 21:53, Victor Shyba ***@***.***> wrote: Hi, sorry for the delay. I'm traveling for some days. I tried using it as a posthook, but it looks like the posthook runs after the response is sent on a thread? At least that is what I have on history from last I tried. I will try again as soon as I'm back. This is the code I saw that looks like running after the response is sent: https://github.com/chihaya/chihaya/blob/7455c2ad4a5860d7f73b98cae4e4160e194fb45b/frontend/udp/frontend.go#L322 This is my middleware implementation: https://github.com/shyba/chihaya/tree/main/middleware/fixedpeer (The other changes on that branch are work in progress, but I plan on upstreaming everything that makes sense when it is done. The one on ports is related to "anonymous mode" where some torrent clients will send 0 or 1 as invalid port to query without announcing) —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[shyba]

Opened issue #569, enjoy the trip!

[mrd0ll4r]

CI is borked, might be fixed via #567, let's wait for that to land.

[mrd0ll4r]

You should be able to rebase this and get the CI to work now :)

[shyba]

Sorry, just saw your message. Rebased, lets see :)

[mrd0ll4r]

pretty sure CI is borked again, but due to something else this time :D Sorry... Not your fault, we'll fix it eventually