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

shared object - ref impl with java/jni #28

Closed
wants to merge 7 commits into from

Conversation

5 participants
@jrhea
Copy link

commented Nov 24, 2018

Resolves #20

@vyzo @bigs @raulk I am not totally done, but it might be worth taking a look at what i have done so far to make sure i am coloring in the lines.

Note: I am a golang neophyte so i apologize in advance for anything that is no idiomatic. Also, let me know if the liberties i took with project/code organization need to be changed.

Build Instructions:

  • install packages dependencies make deps
  • to build the daemon for go make or make go-daemon
  • to build the client for go make go-client
  • to build the daemon for java make java-daemon
  • to build the daemon for java make java-client

Modifications

complete:

  • added build option for an .so and generation of jni source for java (osx only)
  • added a client option to main.go made a separate executable for the client called p2pc
  • added some additional functionality to the goclient to test the java daemon
  • added .gitignore
  • add go and java subfolders to p2pclient
  • control the daemon/client Go code from Java
  • cleanup the domain socket file if client fails
  • .so build command for linux
  • created .so for: daemon only, client only and daemon/client combined

Functionality

i can fire up two daemons on two different domain sockets:

The first one is run from java

$ java p2pd
Control socket: /tmp/p2pd.sock
Peer ID: Qmb3mEWb7JGBxzduizmFnzQ14uYaqsFqmsCU4tssMRY3rT
Peer Addrs:
/ip6/::1/tcp/61459
/p2p-circuit
/ip4/127.0.0.1/tcp/61458
/ip4/192.168.1.129/tcp/61458
/ip4/192.168.2.65/tcp/61458 

the second one from go:

$ p2pd --sock=/tmp/p2pd2.sock
Control socket: /tmp/p2pd2.sock
Peer ID: Qmbr1NfbBMAzhWTxsJUYu1oXXHhSVRx9BW13qR8uMXuhio
Peer Addrs:
/ip6/::1/tcp/59344
/p2p-circuit
/ip4/127.0.0.1/tcp/59343
/ip4/192.168.1.129/tcp/59343
/ip4/192.168.2.65/tcp/59343

Then I can run two clients to interact with each daemon respectively:

client 1 - listen for message

$ java p2pc --command=ListenForMessage 

client 2 - connect to other daemon then send message

p2pc  --command=Connect  --pathc=/tmp/p2c2.sock --pathd=/tmp/p2pd2.sock Qmb3mEWb7JGBxzduizmFnzQ14uYaqsFqmsCU4tssMRY3rT /ip4/127.0.0.1/tcp/61458

p2pc --command=SendMessage --pathc=/tmp/p2c2.sock --pathd=/tmp/p2pd2.sock Qmb3mEWb7JGBxzduizmFnzQ14uYaqsFqmsCU4tssMRY3rT FOOOOOBAAAAAAARR

The message FOOOOOBAAAAAAARR is displayed by client 1

JAVA USERS: you might need to set the java lib path -Djava.library.path=.

@vyzo
Copy link
Collaborator

left a comment

We need a separate binary for the client.

Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved daemon.go
Show resolved Hide resolved p2pclient/go/p2pclient.go Outdated
Show resolved Hide resolved p2pd/main.go Outdated
Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved Makefile Outdated
@jrhea

This comment has been minimized.

Copy link
Author

commented Nov 28, 2018

@vyzo & @mhchia almost done here. These are the final tasks (for this PR):

  • .so build command for linux
  • finish parsing code to pass all command line params from java client to go

i am leaning towards packaging

  • the shared object
  • the java proxy class that interacts with the shared object

into a Java API that exposes all the functionality needed to implement the beacon chain. this would save me the trouble of implementing all the p2pclient code in Java. this pattern might be useful for other beacon chain implementations that don't have libp2p implemented in their project's language.

Thoughts?

@vyzo

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2018

that sounds reasonable -- looking good so far.

Show resolved Hide resolved p2pc/main.go Outdated
Show resolved Hide resolved p2pc/main.go Outdated
@jrhea

This comment has been minimized.

Copy link
Author

commented Nov 29, 2018

@vyzo @raulk @mhchia I believe i addressed all the issues for this PR. Here's a screen cap of it in action:

libp2p-daemon-demo

@jrhea jrhea changed the title shared object with jni shared object - ref impl with java/jni Nov 29, 2018

@jrhea jrhea force-pushed the jrhea:feat/shared-object-java branch from 662ca5a to c2b21e2 Dec 3, 2018

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 3, 2018

@vyzo and @raulk I just rebased from master. What else do you need from me on this PR? There is a lot more work I can add functionality wise, but I was waiting for you guys to signoff on it so far. I will do whatever..just let me know.

@jrhea jrhea referenced this pull request Dec 4, 2018

Closed

Finish p2p Daemon Rebase #43

@vyzo
Copy link
Collaborator

left a comment

this will need a rebase for pubsub.

Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved daemon.go Outdated
Show resolved Hide resolved p2pd/main.go Outdated
@raulk
Copy link
Member

left a comment

A few more comments. This mode of usage for the daemon is welcome. But please bear with us, @jrhea, as we gain consensus around the elements and concepts this PR introduces.

Show resolved Hide resolved p2pclient/java/p2pc.java Outdated
Show resolved Hide resolved p2pc/main.go

jrhea added some commits Nov 24, 2018

- added build option for an .so and generation of jni source for java…
…. added some additional functionality to the goclient to test the java daemon

- added a gitignore and updated the import paths
-split out client into a sep exe called p2pc
-mmacosx-version-min flag is now set dynamically
-switched from ld to gcc linking command and removed ignore unused errors flag.
-prefixed the cgo headers with go- and jni c headers with java- for clarity.
-added a conditional check for OS (darwin or linux) so that the build can be customized for either
- added c scaffolding code for java client. added sep targets for cli…
…ent builds in Makefile.

- finished passing all CL args from so to Go.  remove domain socket on client fail.
Project restructuring. Java code is now for example purposes. Made bi…
…ndings folder for all Makefile and code related to langage bindings

@jrhea jrhea force-pushed the jrhea:feat/shared-object-java branch from c2b21e2 to a18dc15 Dec 9, 2018

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 10, 2018

@vyzo & @raulk I resolved all the issues and reorganized the code I added into a bindings folder. The dir structure now looks like:

-project root
--bindings
---java
----examples

If you type make all it will create:

  • p2pd: The daemon executable
  • p2pc: The client executable
  • libp2pd.dynlib: The daemon shared object with only startDaemon and stopDaemon exported
  • libp2pc.dynlib: The client shared object with only startClient exported
  • libp2p.dynlib: Combine (daemon & client) shared object with startDaemon, stopDaemon and startClient exported. ( I made this so that you don't have to import both .so's)

Please take a look and let me know what you think.

Show resolved Hide resolved test/mock_daemon.go Outdated
Show resolved Hide resolved test/utils.go Outdated
@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 10, 2018

@vyzo thanks for the review. I pushed the changes. I have more functionality to implement in the client side, specifically in p2pclient/p2pc.go

The code only supports these commands:

Identify
Connect
ListenForMessage (NewStreamHandler)
SendMessage (NewStream)

I was holding off on implementing more functionality until you guys OK this mode of operation. That highlights one negative aspect of this mode...going forward there will potentially be one more touch point when functionality is added to the daemon. We will need to make sure it is made available to the shared object. That being said, the other option would be to create a "LibP2P Client Library" where this is housed. Thoughts?

@vyzo

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2018

I am fine with this housed in the daemon repo -- @bigs @raulk thoughts?

@bigs

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2018

yep i would prefer it live within the repo. will do a thorough review tomorrow

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 11, 2018

Thanks @bigs . Sorry in advance for the pain in the ass this review will be 😱

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

@bigs have you been able to take a look at this PR? I'd be happy to jump on a call or chat on gitter about this if it would help.

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 18, 2018

@raulk @mhchia @vyzo @bigs @mvid I drew up some diagrams to share with you guys on the two different ways people can choose to integrate with the libP2P daemon. I am hoping this will clarify what I was thinking when I created this PR.

Option 1 - libP2P.so

Option 2 - libP2Pd.so

@vyzo

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2018

mad drawing skillz!

@raulk

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Hey @jrhea – FINALLY I had time to review this in-depth! Thank you so much for spearheading this effort in the community 🎉

Being only vaguely familiar with Cgo, I would've expected the native interface to mimic the client's public interface. In my head, the design would look like this:

  1. An application in Java, Ruby, C#... calls a void start_client() function on the native interface when starting. This creates a singleton Go client internally and holds a reference to it. We never return the client via the native interface; the return type is void.
  2. We have functions dht_find_peer(), pubsub_subscribe(), connect(), etc. that mirror the Go client's interface and proxy over the call to it, transforming between C types and Go types where needed.
  3. We might need some kind of read_stream(int id) for the application to poll streams. This keeps the IPC behind closed doors and delivers data via the native interface, mimicking the socket API.
  4. When the app shuts down, it calls void stop_client() method that closes the singleton Go client. We'll probably need a int status() method to deterministically enquire the native layer about the status of the Go singleton client.

How does that sound, @jrhea? Happy to collaborate to push this forward.

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 19, 2018

@raulk Thanks for the reply!

Hey @jrhea – FINALLY I had time to review this in-depth! Thank you so much for spearheading this effort in the community 🎉

All good my man. I was happy to do it. I appreciate you guys letting me participate.

An application in Java, Ruby, C#... calls a void start_client() function on the native interface when starting. This creates a singleton Go client internally and holds a reference to it. We never return the client via the native interface; the return type is void.

Totally agree and that is exactly what I would like to discuss on our call. I just implemented the startClient piece with 'magic strings' as commands to prove it works. When I started the PR I could see that the daemon was still early in development and I didn't want to waste too much time nailing down an interface until you guys were ready. I figured that once you guys were ready, we could all agree on what this interface would look like.

We have functions dht_find_peer(), pubsub_subscribe(), connect(), etc. that mirror the Go client's interface and proxy over the call to it, transforming between C types and Go types where needed.

Cool. I think that structs are the only type that might be a problem (unless they are defined in C), but I think we can work around this issue.

We might need some kind of read_stream(int id) for the application to poll streams. This keeps the IPC behind closed doors and delivers data via the native interface, mimicking the socket API.

Yes, exactly. That is probably the key thing to work out. The Java or C# side could either poll for a message received that matches the stream id or it might be cool to just call an externally defined method from Go like ext_stream_rvcd(int id) - sort of like a callback - and that could avoid polling.

When the app shuts down, it calls void stop_client() method that closes the singleton Go client. We'll probably need a int status() method to deterministically enquire the native layer about the status of the Go singleton client.

Perfect!

How does that sound, @jrhea? Happy to collaborate to push this forward.

I like your brain. I think we are on the same page! I really think there is tremendous value in creating a rich interface to other languages so that must of the heavy lifting is done on the Go side. That being said, some teams (e.g. @mhchia ) have already picked the IPC endpoint as their integration point. That is fine though. That is why I had the Makefile create two versions of the shared library:

  • libP2Pd.so (for @mhchia ) that only exposes the control interface
    • startDaemon(config)
    • stopDameon()
  • libP2P.so that exposes the command & control interface:
    • startDaemon(config)
    • stopDameon()
    • startClient(command)

Note: startClient(command) will be changed to a richer interface that you (@raulk ) suggested

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 19, 2018

mad drawing skillz!

Lol thanks! Who said developers can't draw? Amiright? 🎨

@raulk raulk changed the base branch from master to native-api Dec 19, 2018

@raulk

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@jrhea yesterday I updated the target branch for this PR to the native-api long-lived branch, as discussed. We can keep making progress there, until the point where we're confident that the design is stable, and we then we can merge to master.

Notes from the call

Attendees: @jrhea @vyzo @bigs @mkalinin @Nashatyrev @akhila-raju @schroedingerscode @raulk

We see this moving forward as a set of discrete PRs:

  1. One for simple start(config)/stop functions that start an embedded daemon which exposes IPC endpoints. The app interacts with the daemon via IPC. This is what the team wanting to run the daemon on iOS suggested (I think it was somebody from Status.im, but can't remember who).
  2. Laying the foundation for a fully-fledged native API, see #28 (comment). Implementing basic functions like connect, identify.
  3. One PR adding native API calls per subsystem (DHT, conn manager, pubsub, etc.)

We discussed concurrency models, i.e. how to expose native API methods whose Go client counterparts deal with channels. Callbacks and event loops were two suggested models. @jrhea to open issue to discuss.

Instead of creating one glue/translation layer (.so lib) between native API <=> Go client, we can create one per each concurrency model we want to support.

Anything else you guys want to add?

@vyzo

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2018

SGTM

@jrhea

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

@raulk @vyzo @bigs

Anything else you guys want to add?

Great chatting with you guys! I just submitted the first PR in @raulk's list:

Callbacks and event loops were two suggested models. @jrhea to open issue to discuss.

I opened the issue and it can be found:

Looking forward to this. Thanks!

@bigs

bigs approved these changes Jan 7, 2019

Copy link
Collaborator

left a comment

some nits in the C (which i do think should be cleaned) but this looks good

char *arg1 = (char *) 0 ;
(void)jenv;
(void)jcls;
arg1 = 0;

This comment has been minimized.

Copy link
@bigs

bigs Jan 7, 2019

Collaborator

not really sure i understand what's going on here haha. could the preceding lines of this block not be simplified to

char *arg1 = NULL;
char *arg1 = (char *) 0 ;
(void)jenv;
(void)jcls;
arg1 = 0;

This comment has been minimized.

Copy link
@bigs

bigs Jan 7, 2019

Collaborator

same here

@@ -54,6 +57,15 @@ func NewDaemon(ctx context.Context, path string, opts ...libp2p.Option) (*Daemon

go d.listen()

sigc := make(chan os.Signal, 1)
signal.Notify(sigc, os.Interrupt, syscall.SIGTERM)
go func(ln net.Listener, c chan os.Signal) {

This comment has been minimized.

Copy link
@bigs

bigs Jan 7, 2019

Collaborator

oh this is great!

@bigs

This comment has been minimized.

Copy link
Collaborator

commented Jan 7, 2019

sorry this took a thousand years. looking good!

@raulk

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@jrhea – IIUC we should close this PR in favour of #54, correct?

@jrhea

This comment has been minimized.

Copy link
Author

commented Jan 7, 2019

@raulk ya totally. This PR should just be closed and not merged. #54 supersedes it and I will make a new PR to create a native function call interface for native command and native control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.