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

support nif module reload #222

Merged
merged 22 commits into from
Oct 13, 2023
Merged

Conversation

qzhuyan
Copy link
Collaborator

@qzhuyan qzhuyan commented Oct 4, 2023

This PR makes code:purge(quicer_nif) and code:delete(quicer_nif) work properly so that the msquic library could be unloaded from process memory. This is precondition that we could support hot upgrade, unload the old version of code.

There are many changes in test suite to ensure resources are cleaned in each testcase run because it is risky to keep unlean resources running such as calling callbacks with dangling pointer. To prevent that the call to the unload module will wait forever until all resources (msquic handles) are closed.

I also did some major changes to close the msquic handles much earlier when the msquic handles are safe to be closed.
In the old impl. handles are closed only when deallocating nif resources but now we do it much earlier with the cost of using mutex.

Followups:
[] Ensure all APIs are thread safe since we 'close the handle earlier'.
[] {error, invalid_paramter} could be avoided if we check if NULL before passing the handle to msquic API.
[] release script needs to pick versioned so files and lttng build files as well.

@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch from 390315e to 14be7c9 Compare October 6, 2023 14:02
c_src/quicer_connection.c Fixed Show resolved Hide resolved
c_src/quicer_stream.c Fixed Show fixed Hide fixed
c_src/quicer_stream.c Fixed Show fixed Hide fixed
@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch from 14be7c9 to efe9d81 Compare October 6, 2023 20:18
@coveralls
Copy link

coveralls commented Oct 6, 2023

Pull Request Test Coverage Report for Build 6482531202

  • 89 of 105 (84.76%) changed or added relevant lines in 9 files are covered.
  • 16 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.1%) to 83.103%

Changes Missing Coverage Covered Lines Changed/Added Lines %
c_src/quicer_connection.c 20 21 95.24%
c_src/quicer_nif.c 20 21 95.24%
c_src/quicer_reg.c 11 12 91.67%
c_src/quicer_listener.c 8 11 72.73%
c_src/quicer_stream.c 12 17 70.59%
src/quicer_nif.erl 9 14 64.29%
Files with Coverage Reduction New Missed Lines %
c_src/quicer_listener.c 1 85.0%
c_src/quicer_stream.c 1 87.5%
src/quicer.erl 1 88.32%
src/quicer_stream.erl 1 72.22%
c_src/quicer_config.c 3 83.55%
c_src/quicer_nif.c 9 76.76%
Totals Coverage Status
Change from base Build 6473541375: -0.1%
Covered Lines: 3246
Relevant Lines: 3906

💛 - Coveralls

@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch 4 times, most recently from b232e5a to 97f6167 Compare October 10, 2023 07:26
c_src/quicer_ctx.c Outdated Show resolved Hide resolved
c_src/quicer_config.c Outdated Show resolved Hide resolved
c_src/quicer_listener.c Outdated Show resolved Hide resolved
@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch from 0d56b14 to 11067a8 Compare October 10, 2023 11:58
@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch 2 times, most recently from b6886f0 to 3e28c91 Compare October 10, 2023 15:05
@qzhuyan qzhuyan marked this pull request as ready for review October 10, 2023 15:52
build.sh Show resolved Hide resolved
msquic API Doc: StreamOpen.md

So, apps that rely on that event to trigger clean up of the stream **must** handle the case where [StreamStart](StreamStart.md) is either not ever called or fails and clean up directly.
@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch from 662873e to 93686be Compare October 10, 2023 18:53
@qzhuyan qzhuyan force-pushed the dev/william/nif-module-reload branch from 055a455 to dc72431 Compare October 11, 2023 12:31
CMakeLists.txt Show resolved Hide resolved
c_src/quicer_config.c Show resolved Hide resolved
src/quicer_nif.erl Show resolved Hide resolved
It is just nice to call it to avoid unnecessary
return '{error, invalid_parameter}'
thalesmg
thalesmg previously approved these changes Oct 11, 2023
c_src/quicer_nif.c Show resolved Hide resolved
@qzhuyan qzhuyan merged commit d4f3bd3 into emqx:main Oct 13, 2023
26 checks passed
@qzhuyan qzhuyan mentioned this pull request Oct 23, 2023
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.

4 participants