Raise ArgumentError more often #301
Conversation
ruslandoga
force-pushed
the
raise-badarg
branch
from
669fa56
to
78226eb
Compare
| @@ -610,7 +572,8 @@ make_row(ErlNifEnv* env, sqlite3_stmt* statement) | |||
|
|
|||
| columns = enif_alloc(sizeof(ERL_NIF_TERM) * count); | |||
| if (!columns) { | |||
| return make_error_tuple(env, "out_of_memory"); | |||
| // TODO: should be exception, otherwise it's practically invisible | |||
| return make_error_tuple(env, am_out_of_memory); | |||
There was a problem hiding this comment.
make_row and maybe other functions might need to be reworked to return this as an error tuple and not an element of a list of rows.
There was a problem hiding this comment.
I agree with this TODO, it probably should raise. Although what will most likely happen is that it won't be able to raise because the VM cannot allocate enough memory for the exception struct and message regardless. The programmer is going to be in for a world of pain.
But hopefully on the flip side, is that if the VM is able to free some memory, this call unblocks and completes the raise.
There was a problem hiding this comment.
Yeah, that's also something I've been thinking about: would any of the current out_of_memory errors ever surface or would the app just be OOM-killed. I have seen these errors in all of SQLite Erlang/Elixir bindings I could find and in https://github.com/jlouis/enacl as well, but I wonder if those code paths were ever actually executed ...
I think trying to raise is still a good strategy though.
UPDATE: Went to see how enacl is doing it now, and it seems like they have removed alloc_failed atoms in favor of "internal_error": jlouis/enacl@59b9443#diff-43b9ee1b39907dcfc3d096225f5550664a88e3c8a100392e77a0dfb25082a634L171. And also removed many of error tuples in favor of raising exceptions as well.
There was a problem hiding this comment.
would any of the current out_of_memory errors ever surface or would the app just be OOM-killed
That's hard to say because I haven't experienced it yet. I don't know what happens with the VM when you try to allocate a term and there is no memory available. Looking at enif_alloc it returns NULL if it fails to allocate.
Initially my target audience for this library was embedded Nerves devices and memory constrained machines running a small elixir application, so it's probably best to figure out a good strategy for handling out of memory. I suspect, probably the best thing to do is to preallocate with the atoms a term or an exception term to return when there is a memory problem. This in theory should not allocate more memory since we would have pre allocated it prior.
ruslandoga
force-pushed
the
raise-badarg
branch
from
78226eb
to
76b23f9
Compare
Continues #299