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

Prevent meck_procs timeouts when stopping #231

Merged
merged 1 commit into from
May 26, 2021

Conversation

aronisstav
Copy link
Contributor

When stopping (e.g. due to meck:unload/1), meck_procs restore the original module, which may be a time consuming operation.
If a gen_server:call is used for stopping, there's risk of a timeout that will be translated to a confusing error:{not_mocked}.
Using gen_server:stop/1 easily uses an infinity timeout, preventing the issue.

This is fixing a bug where such a time-consuming restoration was causing an end_per_suite operation to fail, confusingly.

@aronisstav
Copy link
Contributor Author

Hmm... it might be the case that gen_server:stop bypasses sys:suspend, which is what the failed test relies on.
A different solution to the same problem is to make 'unload' issue a call with infinite timeout. That will need a custom variant of the "gen_server" function.
Shall I try fixing it that way?

@eproxus
Copy link
Owner

eproxus commented May 17, 2021

Interesting fix. This might be the cause of a few hard-to-reproduce not_mocked errors previously reported.

Can you reproduce the failed test case on your end?

@eproxus
Copy link
Owner

eproxus commented May 17, 2021

Hmm... it might be the case that gen_server:stop bypasses sys:suspend, which is what the failed test relies on.
A different solution to the same problem is to make 'unload' issue a call with infinite timeout. That will need a custom variant of the "gen_server" function.
Shall I try fixing it that way?

That sounds like a good idea!

@aronisstav
Copy link
Contributor Author

Updated!

@eproxus
Copy link
Owner

eproxus commented May 17, 2021

Seems the exception is triggered somehow. Maybe try locally and remove the try catch to see which exception is thrown there. Can you reproduce the failing test cases?

@aronisstav
Copy link
Contributor Author

Now tests pass locally. I had accidentally commited some debug-related code.

@eproxus
Copy link
Owner

eproxus commented May 18, 2021

Looks good. Can you update the CHANGELOG.md too, and squash everything to one commit?

I'd prefer the commit message to be Increase stop timeout to infinity instead, if you agree.

@aronisstav aronisstav force-pushed the patch-1 branch 2 times, most recently from 5137b18 to 33a91b0 Compare May 25, 2021 20:12
@aronisstav
Copy link
Contributor Author

Done!

@eproxus
Copy link
Owner

eproxus commented May 26, 2021

@aronisstav Last minor thing: the changelog entry needs to be under ## [Unreleased]. Sorry for not being clearer about that.

When stopping (e.g. due to meck:unload/1), meck_procs restore
the original module, which may be a time consuming operation.
If a gen_server:call is used for stopping, there's risk of a
timeout that will be translated to a confusing
error:{not_mocked, ...}.

Using gen_server:stop/1 breaks tests, so using an infinity timeout
instead.
@aronisstav
Copy link
Contributor Author

Sorry, fixed again.

@eproxus eproxus merged commit 74a2ac7 into eproxus:master May 26, 2021
@eproxus
Copy link
Owner

eproxus commented May 26, 2021

Thank you! ❤️

@aronisstav aronisstav deleted the patch-1 branch May 26, 2021 15:44
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.

None yet

2 participants