servalproject / serval-dna

The Serval Project's core daemon that implements Distributed Numbering Architecture (DNA), MDP, VoMP, Rhizome, MeshMS, etc.
http://servalproject.org
Other
171 stars 80 forks source link

MDP sockets support (suite) #53

Open rom1v opened 11 years ago

rom1v commented 11 years ago

As this pull request has been closed due to branch renaming, I open a new one.

I just fixed a deadlock issue on the Java part.

But a problem persisted: if a thread would close() a socket, any blocking receive() would not exit as expected. Calling shutdown() instead of close() fixes the problem (commit f93642c).

As it is worth knowing a blocking receive() has failed due to socket close(), commit 8d2ce4b gives the information to the caller (in exception message, like Java DatagramSocket does).

Waiting for your feedbacks...

rom1v commented 11 years ago

I merged your new commits into mdpsock (with quite a lot of conflicts), and modified both your new code to use MDP sockets and my code to use the new type sid_t.

I passed the tests/all (after applying a workaround for bug https://github.com/servalproject/serval-dna/issues/56), only directory_service fails, but it fails on development branch too.

Thus, you should be able to merge in and test without any merge conflicts.

quixotique commented 11 years ago

To run the directory_service tests, you must explicitly make the directory_service executable (it is not included in the default Make target):

make directory_service
./tests/directory_service
quixotique commented 11 years ago

I propose we pull this work into servalproject, and leave it as a feature branch called mdpsock until we have developed some unit tests for the MDP client library, which must pass before merging into mainline development. The tests will have to cover:

rom1v commented 11 years ago

To run the directory_service tests, you must explicitly make the directory_service executable (it is not included in the default Make target)

Thank you. I did it and discovered it was not compiling because it didn't use MDP sockets. Commit 69e7f7a make it compile and tests/directory_service passes.

lakeman commented 11 years ago

I've fetched your branch and rebased it onto our development branch. Squashing, editing and reordering the various changes into a more structured and logical patch series. The low impact changes; 2c6a14d, 4f89a69 & f5fa988 have been pushed to our development branch. The rest of my editing of your changes can be found in a new branch called "mdp" in our github repo. I'd like to see a couple of new test cases that assert that all mdp sockets are cleaned up on graceful closing and abnormal termination. Then there should be no blockers to pushing the global mdp socket to local variable refactoring changes. The JNI bindings should probably be split to a separate pull request for further debate to nail down the design. We need to consider thread safety in more detail before this could be merged.

rom1v commented 11 years ago

Great.

The JNI bindings should probably be split to a separate pull request for further debate to nail down the design.

OK, I used the same pull request because if I had created a new one, GitHub would have included all the commits from the first one (so it would have been more confusing).

However, some JNI bindings commits are already included in your mdp branch (including the main one).

We need to consider thread safety in more detail before this could be merged.

As these changes are client-side, I think there is nothing special to do. The multithreading has to be considered in servald (in my opinion, one unique thread should handle MDP stuff).