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

ReleaseUpdater: refactor to use Process.send_after #27

Merged
merged 1 commit into from
Dec 24, 2016
Merged

ReleaseUpdater: refactor to use Process.send_after #27

merged 1 commit into from
Dec 24, 2016

Conversation

jbodah
Copy link
Contributor

@jbodah jbodah commented Dec 12, 2016

We had a rogue test failing in our test suite after upgrading to Elixir 1.4.0-rc.1 (currently on tzdata 0.5.9 too). After a lot of digging it seems like something is failing due to how Tasks are spawned. I couldn't figure out what the exact issue was, but this refactoring seemed to fix it.

Here's the error I was seeing which was being thrown by the Task.async calls almost immediately in our test.

16:30:27.406 [error] Tzdata.ReleaseUpdater :tzdata_release_updater received unexpected message in handle_info/2: {#Reference<0.0.3.1385>,
 %Task{owner: #PID<0.560.0>, pid: #PID<0.905.0>, ref: #Reference<0.0.5.5243>}}

16:30:27.407 [error] Tzdata.ReleaseUpdater :tzdata_release_updater received unexpected message in handle_info/2: {:DOWN, #Reference<0.0.3.1385>, :process, #PID<0.560.0>, :normal}

It looks like those are being thrown because the Task is trying to send back to the Server process after running, and the server is crashing because it's trying to parse the message as a handle_info: https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/task/supervised.ex#L36. It's a little in the weeds for me, but I think this is generally caught by Task.await, but is being bubbled up to the GenServer implementation instead

This refactor re-uses the existing Server process instead of creating a Task process

@jbodah
Copy link
Contributor Author

jbodah commented Dec 12, 2016

Reproducible by adding a long wait (say 7000ms) in any test in this repo

@lau
Copy link
Owner

lau commented Dec 18, 2016

Hi Josh. Thanks! I'll probably merge this soon after doing some testing.

@venkatd
Copy link

venkatd commented Dec 19, 2016

@lau merging this should resolve #28

@lau lau merged commit d41d56f into lau:master Dec 24, 2016
@lau
Copy link
Owner

lau commented Dec 24, 2016

❤️ 💚 💙 💛 💜

Closes #28

@jbodah
Copy link
Contributor Author

jbodah commented Dec 24, 2016

Thanks @lau

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