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

adding an async function called "shout" #3

Merged
merged 11 commits into from
Jun 20, 2016
Merged

adding an async function called "shout" #3

merged 11 commits into from
Jun 20, 2016

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Jun 17, 2016

Adds an asynchronous function called shout. Here's what it looks like on the JS side:

var HW = new HelloWorld();
HW.shout('hey', { louder: true }, function(err, shout) {
  if (err) throw err;
  console.log(shout); // => hey!
});

Next up:

cc @springmeyer @GretaCB

test('shout error - non string phrase', function(t) {
HW.shout(4, {}, function(err, shout) {
t.ok(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect err to be here, but we are throwing within hello_world.cpp instead of returning an error message. Not sure how to do this. @springmeyer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually on second thought, maybe throwing is best here and the test should be written differently. Not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the current code potentially both throws and returns an error in the callback. So the test is incorrect and should do a try/catch instead and assert the catch is hit.

This "two ways to catch errors" is not ideal and results in extra code in JS of course. But it is not viable to do the validation in the HelloWorld::AsyncShout because one cannot safely copy the data into the threadpool without validating.

Potentially one way to avoid going into the threadpool/still return all errors in the callback would be to call the callback inside HelloWorld::shout instead of throwing errors. I think this should work but I've not tried it. Would be a great thing to look into for this repo because figuring out a cleaner way to do this would benefit the usability of all modules based on this code. Can you ticket for a little later?

try
{
baton->result = return_string;
if (baton->louder)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@springmeyer seeing that the return_string now comes back empty for this method after wrapping it all within the try/catch. Seems odd, but if I pull it out:

/***************** custom code here ******************/

std::string return_string = baton->phrase + "!";

if (baton->louder)
{
    return_string += "!!!!";
}

/***************** end custom code *******************/

try
{
    baton->result = return_string;
}
catch (std::exception const& ex)
{
    baton->error_name = ex.what();
}

things work fine

@mapsam
Copy link
Contributor Author

mapsam commented Jun 18, 2016

All green @springmeyer - this is ready to be merged.

I also added the default option to the Makefile, which just executes npm install --build-from-source

@springmeyer
Copy link
Contributor

Great, looks good to me to merge as well - we can address future improvements as they arise. Notably integrating the results of Project-OSRM/node-osrm#193 (comment), when I understand more.

@mapsam mapsam merged commit 20e1f01 into master Jun 20, 2016
@mapsam mapsam deleted the async branch June 20, 2016 19:36
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.

2 participants