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

exml_event.c: enif_open_resource_type for "parser_pointer" does not specify the destructor function. #8

Merged
merged 2 commits into from
Sep 29, 2014

Conversation

RGafiyatullin
Copy link

The following snippet contains some helper funs I would like to use to demonstrate the issue.
Just paste them into your REPL: https://gist.github.com/RGafiyatullin/675b97dc76c69d8a7c00 .

The leaky case (current esl/master): https://gist.github.com/RGafiyatullin/9cefce33bb22f3350cf0 .
The fixed case (current RGafiyatullin/master): https://gist.github.com/RGafiyatullin/4b895d6282b43d78dc42 .

Bits of reasoning:

  • The mandatory exml_event:free_parser/1 usage is not an option as it is a slightly less expected behaviour rather than a proper resource cleanup upon garbage collection. (The iodevices / ets's / dets's / you-name-it's usually get disposed upon the owner's termination ).
  • exml_event:free_parser/1 being bound to exml_event:reset_parser/1 is IMHO a necessity since any attempt to use exml_event:parse/2 with the resource being previously freed leads to a VM crash. The ErlangVM is not expected to crash with a segfault in case of a mistake in Erlang code.

@RGafiyatullin
Copy link
Author

Ouch. My apologies.
Merging paulgray/master will do the same: https://github.com/paulgray/exml/blob/master/c_src/exml_event.c

@paulgray
Copy link

@RGafiyatullin actually the latest commit is your code ported to the upstream repo.
Good catch!

@michalwski michalwski reopened this Sep 29, 2014
michalwski added a commit that referenced this pull request Sep 29, 2014
exml_event.c: enif_open_resource_type for "parser_pointer" does not specify the destructor function.
@michalwski michalwski merged commit b55e20e into esl:master Sep 29, 2014
@michalwski
Copy link

@RGafiyatullin I've merged your changes into esl/exml master. Many thanks!

michalwski added a commit that referenced this pull request Mar 2, 2018
Enable the use of Escalus as a standalone XMPP client application (without Common Test)
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