Skip to content

Properly cancel timeout on compilation error in ParallelCompiler #7428 #7429

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

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

allansson
Copy link
Contributor

The :timed_out message was not cancelled properly when compilation failed in ParallelCompiler. This meant the timeout message would be delivered even after compilation completed. This could be problematic when invoking compilation programmatically via e.g. Mix.Task.run("compile", ["--return-errors"]) since it means that the calling process, depending on the implemenation, might crash unexpectedly or get its inbox filled with messages.

The fix simply calls the cancel_waiting_timer when a compilation error has ocurred in the same way it is called in other similar cases.

This fixes issue #7428

…ir-lang#7428

The :timed_out message was not cancelled properly when compilation failed in ParallelCompiler. This meant the timeout message would be delivered even after compilation completed. This could be problematic when invoking compilation programmatically via e.g. Mix.Task.run("compile", ["--return-errors"]) since it means that the calling process, depending on the implemenation, might crash unexpectedly or get its inbox filled with messages.

The fix simply calls the cancel_waiting_timer when a compilation error has ocurred in the same way it is called in other similar cases.
@josevalim
Copy link
Member

@allansson can you see if it is possible to check for this behaviour in a test? The test suite is here and we already have failing tests for the parallel compiler, maybe one of those have a leaked message that will be fixed with your test? It should be a matter of calling refute_received {:timed_out, _}. Thanks!

@allansson
Copy link
Contributor Author

Of course! I did look at the tests and considered implementing one, but decided not to because there can be a considerable delay between compilation finishing and the timeout message being received. I didn't want to create a test which would have to run for a at least 10 seconds or so.

I was not aware of the refute_received assertion. It will make things really easy.

I'll try to fix at soon as possible. While I am at it, I suppose could write some tests for when compilation is successful as well. ^.^

@josevalim
Copy link
Member

josevalim commented Mar 8, 2018 via email

@allansson
Copy link
Contributor Author

Ok. I have almost completed writing two tests.

I can get around the long wait by setting the long_compilation_threshold option to something low. But that means that if compilation was to stall for too long for whatever reason, the tests would fail.

If it was possible to "spy" on calls to send_after and cancel_timer I could of course verify that all references are cleared up after execution has finished, but I am not aware of any way of doing so?

@josevalim josevalim merged commit 24790a8 into elixir-lang:master Mar 8, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

josevalim pushed a commit that referenced this pull request Mar 9, 2018
#7429)

The :timed_out message was not cancelled properly when compilation failed in ParallelCompiler. This meant the timeout message would be delivered even after compilation completed. This could be problematic when invoking compilation programmatically via e.g. Mix.Task.run("compile", ["--return-errors"]) since it means that the calling process, depending on the implemenation, might crash unexpectedly or get its inbox filled with messages.

The fix simply calls the cancel_waiting_timer when a compilation error has ocurred in the same way it is called in other similar cases.

Signed-off-by: José Valim <jose.valim@plataformatec.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants