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

Fix (really unlikely) 'count' bug #5

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

riga
Copy link
Contributor

@riga riga commented Jun 14, 2013

The reset should take place after the remote call to keep the counters consistent.

I'm porting your rpc-stream idea over to python and stumbled over this 'bug' (but I never had to reset the counter....so no beer for me ;) ).

edit: The Python port works, see https://github.com/riga/rpyc-stream

riga added 4 commits June 14, 2013 23:04
The reset should take place _after_ the remote call.
Now, local callbacks are called via apply. This is necessary
when e.g. wrapping around node modules.
Example:
var server = rpc({
  foo: function(arg1, arg2, arg3, cb) {
    // do something with all args
    cb(null, arg1+arg2+arg3);
  }
});
var client = rpc();

client.pipe(fserver).pipe(client);

client.rpc('foo', [1, 2, 3], function(err, res) {
  // res will be 6
});

Before, the foo function only received 2 arguments: a list with all
arguments and the callback. This was not useful when wrapping around node modules, e.g. fs.
No responses were emitted.
@dominictarr
Copy link
Owner

hi, I merged the first two commits, but not riga@3699289 because it broke the tests - I used to have it that way,
but decided it was better to pass an array of args, because otherwise you end up doing [].slice.call(arguments) multiple times per op, and doing it once is slow.

@riga
Copy link
Contributor Author

riga commented Jun 17, 2013

Thanks for merging!
There was a bug in riga@3699289 that I fixed in riga@b3c58c2.

I see your point with the [].slice.call(arguments) thing, but this way e.g. in your example where you wrap your rpc object around the fs module, all calls to functions without the (someValue, cb) signature will fail.

@dominictarr
Copy link
Owner

oh, hmm - that example is out of date... I should update that.

I can't merge the rest of this though, because it's a breaking change, and this api is being used in multilevel -- multilevel is the most important use case for rpc-stream

maybe if you add another method that is a short cut for that kind of thing?

@riga
Copy link
Contributor Author

riga commented Jun 18, 2013

Sorry for making 2 commits to solve this. riga@74defea now fixes the bug you mentioned in riga@3699289.

FYI: we plan to use rpc-stream and rpyc-stream for distributed handling & processing of physics analyses at our institute.

edit: By the way, what confused me was the hello, JIM usage example in your readme. The args name, cb imply that name is a string although it's an array. It still works since JS knows what to do with "hello, " + ["JIM"]...

@dominictarr
Copy link
Owner

cool! real science! can you tell me more?

@riga
Copy link
Contributor Author

riga commented Jun 18, 2013

Software installation/dependencies and software, resource and data availability are major questions in physics.
That's why we are developing a web app that allows you to create and manage analyses via a standard browser.
Our server connects to selected workers (or even hpc grids) over ssh and therefore uses RPC for communication.
We need the Python port because most of our code is written in Python/C++, but since your stream/pipe concept is
both lightweight and performant I also plan to port it to C++ or Java.
Our research interests are elementary and astro particle physics (top quark & higgs physics, cosmic magnetic fields) .... and of course computing ;)

@marcelklehr
Copy link

just stumbled upon that args issue. I'd need to wrap my modules by hand to be able to call the remotely, now?

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

Successfully merging this pull request may close these issues.

3 participants