Inconsistency with NIF

162 views
Skip to first unread message

Vignesh Rajagopalan

unread,
Nov 26, 2015, 7:21:13 AM11/26/15
to elixir-lang-talk
Hi everyone,

I'm currently working on writing an elixir binding to libgraphqlparser here at aarvay/graphql_parser. I ran into some inconsistency in the way NIF works when I added tests against it. Here's the issue.

Quoting the issue: "When parse/1 is called several times with the same valid input, it errors out sometimes." An example to reproduce the inconsistency is commented out in the issue.

This is the first time I'm writing a NIF and so I might have messed up something (I'm guessing, in the way I handle binaries). Can someone help me out with is?

PS: Does the ':' or '$' character affect ?

Thanks,
Vignesh

David Whitlock

unread,
Nov 26, 2015, 10:31:46 PM11/26/15
to elixir-lang-talk
Hi,

I think you need to find out what calls the NIF is making to the underlying graphql library -- a naive, but effective, way of doing this is to add several printf calls to the NIF! All the  errors are coming from the graphql library, and that knowledge might help you debug it.

Let us know how you get on,
David

Vignesh Rajagopalan

unread,
Nov 27, 2015, 4:00:26 PM11/27/15
to elixir-lang-talk
David,

Thanks for the suggestion. It revealed something. This is the first call I'm making to the underlying graphql library:

struct GraphQLAstNode *graphql_parse_string(const char *text, const char **error);

text is the input I'm getting from parse/1 and I'm allocating a binary for it by calling enif_inspect_binary(env, argv[1], &graphql_data)

when i printf graphql_binary->data I'm getting additional content along with what I pass :confused:

iex)> GraphQL.Parser.parse "{ field(complex: { a: { b: [ $var ] } }) }"
GOT : "{ field(complex: { a: { b: [ $var ] } }) }build/d"

iex> GraphQL.Parser.parse "{ field(complex: { a: { b: [ $var ] } }) }"
GOT : "{ field(complex: { a: { b: [ $var ] } }) }@"

Ben Wilson

unread,
Nov 27, 2015, 4:14:47 PM11/27/15
to elixir-lang-talk
Just out of curiosity, what would be the advantage of using this wrapper around a C++ lib instead of using one of the existing elixir graphql parsers?

Bruce Williams

unread,
Nov 27, 2015, 4:26:31 PM11/27/15
to elixir-l...@googlegroups.com
Ben,

Mainly, I'd guess, because libgraphqlparser is an officially supported GraphQL project. Presumably less maintenance when the spec changes, etc.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-talk" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-ta...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-talk/3ce56e60-4358-431e-ad53-caf4cb0a4796%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Vignesh Rajagopalan

unread,
Nov 27, 2015, 4:28:17 PM11/27/15
to elixir-lang-talk
Ben,

I suspect this is likely to be the best choice of parser in the long run, due to a few reasons:

 [1] This library is officially supported by Facebook and they have exposed C API's for writing bindings instead of writing a parser from scratch.
 [2] Resulting AST dump needs to match across platforms.
 [3] GraphQL spec changes continuously and bindings would be easier to update (API changes are small compared to changing grammar written in your programming language)
 [4] Other languages like ruby have adopted doing the same thing.

David Whitlock

unread,
Nov 28, 2015, 5:36:39 PM11/28/15
to elixir-lang-talk
This is an example of a NIF I've written. I'm wondering if you need the call to enif_alloc_binary (on line 68) to guarantee the size of the binary.
Let us know how you get on, and good luck!


On Thursday, November 26, 2015 at 7:21:13 PM UTC+7, Vignesh Rajagopalan wrote:

Jim Freeze

unread,
Nov 29, 2015, 7:18:19 PM11/29/15
to elixir-l...@googlegroups.com
Vignesh

Have you checked that you have spaced allocated to null terminate your cstrings?

I see that you are using enif_alloc_binary. I have been using enif_alloc, but I do allocated enough room for the '\0' terminater. 
Not sure if enif_alloc_binary does that. 

enif_alloc_binary(error_len, &output_binary);


--
You received this message because you are subscribed to the Google Groups "elixir-lang-talk" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-ta...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.



--
Dr. Jim Freeze, Ph.D.

Vignesh Rajagopalan

unread,
Nov 30, 2015, 5:15:27 PM11/30/15
to elixir-lang-talk
Jim,

Thanks a lot. That was indeed the problem but present in a different way. I ended up passing a null terminated string to the NIF which ensured the binary is read properly. Tests pass now :) Phew.

Thanks everyone!

Jim Freeze

unread,
Nov 30, 2015, 5:29:25 PM11/30/15
to elixir-l...@googlegroups.com
Hi, Vignesh.

Glad I could be of some help.

I didn't think about adding the null to the elixir string. Is there a chance that that you will get repeated null terminations unless you check that the string already is null terminated?

For my situation, I do all this work in the c code. Here is what I have done.

char *
alloc_and_copy_to_cstring(ErlNifBinary *string)
{
    char *str = (char *) enif_alloc(string->size + 1);
    strncpy(str, (char *)string->data, string->size);
    str[string->size] = 0;
    return str;
}

And it is called with code as below:

    ErlNifBinary string;

    if (argc != 1 || ! enif_inspect_binary(env, argv[0], &string))
      return enif_make_badarg(env);

    char *str = alloc_and_copy_to_cstring(&string);





For more options, visit https://groups.google.com/d/optout.

Vignesh Rajagopalan

unread,
Dec 1, 2015, 5:44:59 AM12/1/15
to elixir-lang-talk
Jim,

Although I don't understand how repeated null terminations could creep in, this seems like the "right" solution. Appending null termination along with the elixir string ensures no reads happen after that address. But it smells hacky.

Thanks once again. I guess I was just afraid to write more C code :P Gotta brush up my C skills.
Reply all
Reply to author
Forward
0 new messages