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

Add support for tunnel.ping RPC #221

Merged
merged 2 commits into from
Nov 25, 2022
Merged

Conversation

KyleMaas
Copy link
Contributor

Don't know if this is the correct way to do this, but this adds support for the tunnel.ping RPC from the https://github.com/ssbc/ssb-tunnel package that ssb-browser-demo uses.

@mycognosist
Copy link
Member

Thanks for the PR! I'll leave @decentral1se to take a look at the code.

My one suggestion would be to add an entry for this to manifest.go:

"tunnel": {

@mycognosist mycognosist added the enhancement New feature or request label Nov 22, 2022
Copy link
Member

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @KyleMaas! I'm not familiar with this endpoint but trust you need it for something. Do you have any links / refs for it? Np if not, just curious what you're planning 😸

It'd be great to have a test for this which I don't think would be hard to add, since the return is quite simple, no mocking etc. Just hit the endpoint and make sure you get back a datetime... could you also manage it? Feel free to ask any questions! 🙏 💯

#221 (comment) is a great suggestion too!

@KyleMaas
Copy link
Contributor Author

@decentral1se It was used in ssb-browser-core to keep the browser from closing connections. See:

https://github.com/arj03/ssb-browser-core/blob/da9dd68fa945d192b40101b579153414f154c985/net.js#L78

We also ran into it in this thread:

%/v6QNpeO0D7M6ayz05PE/fuhWvN4zkOFPh3pFQ4Pqzo=.sha256

Looking at the code, I think writing a test for this is beyond my current skill level. This project is my first time working with Go, and I am not particularly familiar with this codebase, either. If there's an example you could point me to of how to best test an RPC, that'd be great. I looked for any other tunnel tests that I could base a new test off of, like for tunnel.isRoom, and couldn't find one.

One other question I had on this is that the original tunnel.ping was set up as a sync RPC, but I did not see any way to do that with the Go version, so I marked it as async. I don't know enough about the RPC system to know the ramifications of that, but it did seem to work with ssb-browser-demo so I figured it was probably okay.

@decentral1se
Copy link
Member

@KyleMaas I'll cover on the test, I think it might not be obvious given that there are a few moving parts that need to come together for this new test file. I'll ping you when I have something working. For the Sync/Asyne issue, I'm also still trying to figure out that! If you're getting working request/response calls, then let's assume we're good for now 🙃 Thanks for diving into Go coding, we need all the help we can get!

@decentral1se decentral1se merged commit 1f52f69 into ssbc:master Nov 25, 2022
@KyleMaas
Copy link
Contributor Author

Cool. I can't verify for sure that ssb-browser-core is getting "proper" responses to the RPC considering it pretty much just ignores what it gets back. But it does seem to be getting a response, which is really all it needed. I don't even know what else uses that RPC, so maybe the response format is actually irrelevant and the only thing it's actually used for is to test for whether or not it gets a response at all. Tests would be ideal to know for sure.

Thanks!

@KyleMaas KyleMaas deleted the add-tunnel-ping branch November 25, 2022 22:08
@decentral1se
Copy link
Member

decentral1se commented Nov 25, 2022

@KyleMaas

OK, after looking a bit deeper, I think we may have mixed up some terms. The original https://github.com/ssbc/ssb-tunnel goals was peering via a pub. However, the tunnel.* endpoints in go-ssb were added to in #92 for peering via a room.

So, I think this "works" in the sense that it will keep your connections alive by returning something and looks like the MUXRPC call you need. But we've mixed up the tunnel.* rooms namespace because tunnel.ping now doesn't ping from go-ssb to a go-ssb-room to check the connection is up, it just responds to keep up a go-ssb connection 🤯

I hope this is making sense. I'm inclined to leave this as-is, to support your use-case / experimenting and I will open an issue to track fixing this proper. I'm not sure what the real fix is after all...

@KyleMaas
Copy link
Contributor Author

@decentral1se

Wouldn't it still be applicable, though, since this project is a replacement for pubs and that was the original purpose of ssb-tunnel? If a pub had ssb-tunnel installed before, the tunnel.ping call would be responded to by the pub itself, which is exactly what this does now. It didn't run the RPC through a room attached to a pub before, and it still doesn't. Perhaps @arj03
could chime in here regarding its usage in ssb-browser-core, but I'd argue that the actual implementation of the RPC here is consistent with the RPC of ssb-tunnel when it was installed on a pub before. And if you didn't have ssb-tunnel installed before, you wouldn't have the other tunnel.* RPCs, which are implemented here. So from my perspective, I don't think it's out of place.

@arj03
Copy link
Member

arj03 commented Nov 26, 2022

I do think it's fine to have the ping method here as well to make it consistent with a js pub that has ssb-tunnel installed. One thing that is a bit confusing is that there are 2 ping methods with different behaviors. As Kyle says, the tunnel.ping is really important for a browser client connecting to keep the connection alive, so it's really nice to see it added here. Also tunnel should not be only specific to rooms, it is nice that you can potentially tunnel through a pub as well.

@KyleMaas
Copy link
Contributor Author

@arj03

Can you elaborate more on "there are 2 ping methods with different behaviors" please? Because at this point I'd think we might as well harmonize them.

@KyleMaas
Copy link
Contributor Author

Ah, I see:

https://github.com/ssbc/go-ssb-room/blob/00a1452cfcf2fdea55f95909ca053eb35e97a4e2/muxrpc/handlers/tunnel/server/state.go#L83

That returns a numeric value, but it's calculated differently. Which one is correct?

@decentral1se
Copy link
Member

decentral1se commented Nov 28, 2022

My worry is that we have isRoom / connect which speak to rooms and ping which speaks to a local pub. They all live under the namespace of "tunnel" but do quite different things (querying rooms vs. pubs). Maybe that's fine, I'm pretty late to the MUXRPC design party. Maybe we should be doing a documentation sprint on both the go-ssb / go-ssb-rooms MUXRPC endpoints?

@KyleMaas
Copy link
Contributor Author

Shouldn't both rooms and pubs have isRoom, connect, and ping?

@decentral1se
Copy link
Member

I honestly don't know. isRoom on a pub? But that's not really my main point. The logic of isRoom on go-ssb right now is not confirming if go-ssb itself is a room or not, it is forwarding a request on to a room. But the logic of ping on go-ssb doesn't forward the request on to a room, it just responds a ping from the pub. The semantics of the calls are very different. I'm really not familiar with MUXRPC but this seems confusing and not intuitive. So again, maybe we just need docs?

Maybe we can continue on #231? Thanks.

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

Successfully merging this pull request may close these issues.

None yet

4 participants