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

Check container recreation in OnBackgroundActivated #2633

Merged
merged 3 commits into from
Nov 5, 2018

Conversation

bartlannoeye
Copy link
Contributor

PR checklist

Quick summary of changes

  • Which issue does this PR relate to? Fixes [Prism] Background activation should not recreate container #2632
  • Anything that requires particular review or attention? Check explanation in issue to see if I forgot something.
  • Do all automated tests pass? No reference found in tests, didn't rerun whole suite.
  • Have automated tests been added for new features? No
  • Have you raised issues for any needed follow-on work? No
  • Have docs been updated? No reference found
  • If breaking changes or different ways of doing things have been introduced, have they been communicated widely? This probably should be added to the changelog for existing apps with Prism and background tasks.

@crutkas
Copy link
Member

crutkas commented Oct 8, 2018

this should be on the main readme i think.

@sibille
Copy link
Collaborator

sibille commented Oct 9, 2018

Thanks Bart!! Did a bit of research where this came from, and saw this was initially added in PR #1606, won't this lead to regression in case the app is terminated?

@bartlannoeye
Copy link
Contributor Author

Since the background task is in-process, it's terminated as well and can't re-activate a terminated app. Am I correct?

I've tested following cases (manually through VS lifecycle states):

  • background task (on running app)
  • suspend - resume
  • suspend - background task - resume
  • suspend and shutdown - launch

All worked fine.

@bartlannoeye
Copy link
Contributor Author

Looks like there's one more step to be tested (see issue). I'll look at it tonight, can stay open for now.

@bartlannoeye
Copy link
Contributor Author

Quick update: making progress on this, but checking a few more items to see for the best solution. Out of town for 2 days, so picking this up again over the weekend.

@sibille
Copy link
Collaborator

sibille commented Oct 29, 2018

@bartlannoeye, what's the status for this pr?

Best thing we can do for now, focusing our time on v7 to fix this correctly.
@bartlannoeye
Copy link
Contributor Author

Hey @sibille, the complexity of trying to make it work with the current release, combined with multiple events (attending and/or speaking) resulted in taking a bit longer. I've done a "best-effort" fix which handles 95% of all use cases and described the last edge case (which should rarely happen) in the linked issue. We'll have to take this last one in our next Prism release.

It's an improvement over the current code in WTS, so if the team agrees, I would suggest to merge this PR and we'll do our best to get the next release out the door asap.

@bartlannoeye bartlannoeye changed the title Remove container recreation in OnBackgroundActivated Check container recreation in OnBackgroundActivated Nov 5, 2018
@sibille
Copy link
Collaborator

sibille commented Nov 5, 2018

Thanks @bartlannoeye, sounds good to me!

@sibille sibille merged commit 6689ad8 into microsoft:dev Nov 5, 2018
@bartlannoeye bartlannoeye deleted the Issue2632-PrismActivation branch November 5, 2018 16:07
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