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

MDP sockets support (suite) #53

Open
wants to merge 29 commits into
base: development
from

Conversation

Projects
None yet
3 participants
@rom1v
Contributor

rom1v commented Feb 21, 2013

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 added some commits Nov 6, 2012

Make overlay_mdp_client_socket_path local
There is no reason for this variable to be external. It is used in init() and
was used in done(), but the unlink() in done() is useless: it was already called
on init() so the file was already unlinked.
MDP sockets support
Provide MDP sockets instead of using a global one.
Remove useless external declaration
I removed the definition of mdp_client_socket, but an external declaration was
remaining.
Fix broken dna lookup
lookup_send_request() must use the socket of the caller, not create a new
one (detected by dnaprotocol test script).
overlay_mdp_send must be blocking
Else, once the buffer is full, we get errno=11
Missing free for a JNI buffer
ReleaseByteArrayElements last parameter is mode. Its value is:
 - 0: copy back the content and free the buffer;
 - JNI_COMMIT: copy back the content and *do not* free the buffer;
 - JNI_ABORT: free the buffer without copying back the possible changes.

 We definitely want 0.

 With JNI_COMMIT, after receiving more than ~1024 packets, it crashes:
   ReferenceTable overflow (max=1024)
Merge branch 'master' into mdpsock
Conflicts:
	commandline.c
	mdp_client.c
	serval.h
	sourcefiles.mk
Fix compilation warnings
(unused, signed/unsigned variables...)
Remove question-comment
Using fd+1 is the right way to use select()
Remove sun_path file on MDP socket close
Avoid to keep a lot of useless files like:
  var/serval-node/mdp-client-XXXX-XXXXXXXX.socket
Delete all socket files on servald start
If serval does not close properly, socket files are kept in
/data/data/org.servalproject/var/serval-node. Therefore, we need to clean up
when servald starts.
shutdown() socket instead of close()
Makes any blocking recv exit
Special return code for "socket closed"
Blocking recv calls return a special return code (-2) for notifying the socket
is closed (this information is useful for MDP socket users).

This implementation is unsatisfactory: return code should instead be consistent
with recvfrom(), that is always returning the length of received data (0 for
socket closed).

@rom1v rom1v referenced this pull request Feb 21, 2013

Closed

MDP sockets support #39

@rom1v

This comment has been minimized.

Contributor

rom1v commented Mar 6, 2013

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 #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

This comment has been minimized.

Member

quixotique commented Mar 7, 2013

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

This comment has been minimized.

Member

quixotique commented Mar 7, 2013

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:

  • compilation of some simple MDP client C source code which communicates successfully with a Serval DNA daemon and a remote client
  • ditto for Java source code
@rom1v

This comment has been minimized.

Contributor

rom1v commented Mar 7, 2013

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.

rom1v added some commits May 31, 2013

Merge branch 'development' into mdpsock
Conflicts:
	commandline.c
	mdp_client.c
	serval.h
Merge branch 'development' into mdpsock
Conflicts:
	commandline.c
	mdp_client.c
	serval.h

@rom1v rom1v referenced this pull request Aug 19, 2013

Open

Rhizome parallelization #68

@lakeman

This comment has been minimized.

Member

lakeman commented Aug 26, 2013

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

This comment has been minimized.

Contributor

rom1v commented Aug 29, 2013

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).

@lakeman lakeman force-pushed the servalproject:development branch from c7fde5e to 2296196 Oct 1, 2014

@lakeman lakeman force-pushed the servalproject:development branch from 35e368f to 0c275e6 Oct 19, 2015

@quixotique quixotique force-pushed the servalproject:development branch from 62ce14f to 463e276 Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment