Details

    • Patch Info:
      Patch Available

      Description

      There is no way as far as I can tell for node.js servers to throw thrift exceptions.

      I have made a patch to allow it to throw exceptions. It lets the node.js server implementation give params directly to the result object, thereby being able to specify the exception. It doesn't affect normal (non exception) return data.

      Test case: https://gist.github.com/1151782
      Install thrift module "npm install thrift", generate thrift "thrift --gen js:node test.thrift" and run server then client.

      1. nodejs-exception.patch
        1 kB
        Hans Duedal
      2. thrift-1267-callback.patch
        2 kB
        Hans Duedal
      3. thrift-1267-callback-ns-fix.patch
        2 kB
        Hans Duedal
      4. package.json
        0.8 kB
        Henrique Mendonça
      5. server.js
        5 kB
        Henrique Mendonça
      6. client.js
        4 kB
        Henrique Mendonça
      7. Makefile
        1 kB
        Henrique Mendonça
      8. THRIFT-1267-ex-ns-fixes+tests.patch
        7 kB
        Henrique Mendonça

        Issue Links

          Activity

          Hide
          Hans Duedal added a comment -

          Applies to both 0.7.x and trunk atm.

          Show
          Hans Duedal added a comment - Applies to both 0.7.x and trunk atm.
          Hide
          Wade Simmons added a comment -

          I wonder if we should instead make a backwards incompatible change for the 0.8 release and change the callback style to match the rest of Node.js: callback(err, result). instead of the current: callback(result).

          Show
          Wade Simmons added a comment - I wonder if we should instead make a backwards incompatible change for the 0.8 release and change the callback style to match the rest of Node.js: callback(err, result). instead of the current: callback(result).
          Hide
          Paul Querna added a comment -

          +1 to incompatible change to use the node.js style of callback(err, result); All of our existing code uses this style, and it is essential when dealing with async handlers in a service.

          Show
          Paul Querna added a comment - +1 to incompatible change to use the node.js style of callback(err, result); All of our existing code uses this style, and it is essential when dealing with async handlers in a service.
          Hide
          Wade Simmons added a comment -

          It looks like Russell Haering has a good example of how this should look:

          https://gist.github.com/1098395

          I'll start working on a patch for this next week if no-one jumps in.

          Show
          Wade Simmons added a comment - It looks like Russell Haering has a good example of how this should look: https://gist.github.com/1098395 I'll start working on a patch for this next week if no-one jumps in.
          Hide
          Hans Duedal added a comment -

          New patch, with callback(err, result) instead.
          Updated gist:1151782 usecase also.
          Should support syntax from gist:1098395 (Russel Haering) as well.

          Implemented it using instanceof and early returns. What do you think?

          Show
          Hans Duedal added a comment - New patch, with callback(err, result) instead. Updated gist:1151782 usecase also. Should support syntax from gist:1098395 (Russel Haering) as well. Implemented it using instanceof and early returns. What do you think?
          Hide
          Hans Duedal added a comment -

          New version. The previous one didn't get the namespace right for exceptions that where included from other files.

          Show
          Hans Duedal added a comment - New version. The previous one didn't get the namespace right for exceptions that where included from other files.
          Hide
          Victor Powell added a comment -

          I'm new to this project but this is a show stopper for Node.JS usage. I dont consider it a priority to support backwards compatibility with the old mechanism or waiting until 0.8 release to commit this fix.

          Show
          Victor Powell added a comment - I'm new to this project but this is a show stopper for Node.JS usage. I dont consider it a priority to support backwards compatibility with the old mechanism or waiting until 0.8 release to commit this fix.
          Hide
          Jake Farrell added a comment -

          Hans, can you please add the test cases into the lib/test for this

          Show
          Jake Farrell added a comment - Hans, can you please add the test cases into the lib/test for this
          Hide
          Henrique Mendonça added a comment - - edited

          I was trying to extend Roger's new test.sh to use a nodejs server but I noticed that the compiler wasn't working any more... no exceptions, namespaces, etc. I've also create a first draft of a test server (see attached test/nodejs/server.js)
          under thrift/test run:

          ../compiler/cpp/thrift --gen js:node -o ./nodejs ThriftTest.thrift
          export NODE_PATH=../lib/nodejs/lib/thrift/
          node nodejs/server.js
          

          I had to re-base this old patch 'thrift-1267-callback-ns-fix.patch' and make some other small changes. Please someone have a look/test and commit this. Otherwise we won't be able to use nodejs.

          That's what I was trying to archive, if anyone wants to have a look, I think there is still something missing since it doesn't seem to close the stream properly... (I only see the first test)
          test/test.sh:

          do_test "cpp-nodejs" "binary" "buffered-ip" \
                  "cpp/TestClient" \
                  "node nodejs/server.js" \
                  "1"
          

          Thanks

          Show
          Henrique Mendonça added a comment - - edited I was trying to extend Roger's new test.sh to use a nodejs server but I noticed that the compiler wasn't working any more... no exceptions, namespaces, etc. I've also create a first draft of a test server (see attached test/nodejs/server.js ) under thrift/test run: ../compiler/cpp/thrift --gen js:node -o ./nodejs ThriftTest.thrift export NODE_PATH=../lib/nodejs/lib/thrift/ node nodejs/server.js I had to re-base this old patch 'thrift-1267-callback-ns-fix.patch' and make some other small changes. Please someone have a look/test and commit this. Otherwise we won't be able to use nodejs. That's what I was trying to archive, if anyone wants to have a look, I think there is still something missing since it doesn't seem to close the stream properly... (I only see the first test) test/test.sh: do_test "cpp-nodejs" "binary" "buffered-ip" \ "cpp/TestClient" \ "node nodejs/server.js" \ "1" Thanks
          Hide
          Roger Meier added a comment -

          Would be great to add all that stuff!
          I'm currently not familiar with node...

          I have the same issue with the testcase, just shows the first test case.
          The other thing is, that the [^THRIFT-1267-ns-fixes.patch] breaks the examples located at lib/nodejs/examples/

          Show
          Roger Meier added a comment - Would be great to add all that stuff! I'm currently not familiar with node... I have the same issue with the testcase, just shows the first test case. The other thing is, that the [^THRIFT-1267-ns-fixes.patch] breaks the examples located at lib/nodejs/examples/
          Hide
          Henrique Mendonça added a comment -

          OK thanks for the feedback Roger,
          a line too much in the compiler and the client were outdated (see comments above). The example should work again now.

          I'm also adding the 4 files I have under test/nodejs, including a package.json but it wasn't tested with npm.

          The client.js is just a start, it only test a 12 of the services and it would look a lot nicer with qunit, but it'd add a dependency...

          From these few unit tests we can see already that a lot of stuff is not working as it should, e.g. int64 (negative and large values), maps and services with exceptions only work partially (app. exceptions come as 'null' and nothing comes back on success)

          Anyway, I think that's an improvement already.

          Show
          Henrique Mendonça added a comment - OK thanks for the feedback Roger, a line too much in the compiler and the client were outdated (see comments above). The example should work again now. I'm also adding the 4 files I have under test/nodejs, including a package.json but it wasn't tested with npm. The client.js is just a start, it only test a 12 of the services and it would look a lot nicer with qunit, but it'd add a dependency... From these few unit tests we can see already that a lot of stuff is not working as it should, e.g. int64 (negative and large values), maps and services with exceptions only work partially (app. exceptions come as 'null' and nothing comes back on success) Anyway, I think that's an improvement already.
          Hide
          Roger Meier added a comment -

          Yes, this is a improvement => committed!
          Thanks Henrique!

          Show
          Roger Meier added a comment - Yes, this is a improvement => committed! Thanks Henrique!
          Hide
          Hudson added a comment -

          Integrated in Thrift #385 (See https://builds.apache.org/job/Thrift/385/)
          THRIFT-1267 Node.js can't throw exceptions
          Patch: Henrique Mendonca

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1230797
          Files :

          • /thrift/trunk/.gitignore
          • /thrift/trunk/compiler/cpp/src/generate/t_js_generator.cc
          • /thrift/trunk/configure.ac
          • /thrift/trunk/lib/nodejs/examples/Makefile
          • /thrift/trunk/lib/nodejs/examples/server.js
          • /thrift/trunk/lib/nodejs/examples/server_multitransport.js
          • /thrift/trunk/test/Makefile.am
          • /thrift/trunk/test/nodejs
          • /thrift/trunk/test/nodejs/Makefile.am
          • /thrift/trunk/test/nodejs/client.js
          • /thrift/trunk/test/nodejs/package.json
          • /thrift/trunk/test/nodejs/server.js
          • /thrift/trunk/test/test.sh
          Show
          Hudson added a comment - Integrated in Thrift #385 (See https://builds.apache.org/job/Thrift/385/ ) THRIFT-1267 Node.js can't throw exceptions Patch: Henrique Mendonca roger : http://svn.apache.org/viewvc/?view=rev&rev=1230797 Files : /thrift/trunk/.gitignore /thrift/trunk/compiler/cpp/src/generate/t_js_generator.cc /thrift/trunk/configure.ac /thrift/trunk/lib/nodejs/examples/Makefile /thrift/trunk/lib/nodejs/examples/server.js /thrift/trunk/lib/nodejs/examples/server_multitransport.js /thrift/trunk/test/Makefile.am /thrift/trunk/test/nodejs /thrift/trunk/test/nodejs/Makefile.am /thrift/trunk/test/nodejs/client.js /thrift/trunk/test/nodejs/package.json /thrift/trunk/test/nodejs/server.js /thrift/trunk/test/test.sh

            People

            • Assignee:
              Henrique Mendonça
              Reporter:
              Hans Duedal
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development