[AIRFLOW-703] Stop Xcom being cleared too early

[blrnw3]

Only clear Xcom when the task is CERTAIN to start executing
Fixes AIRFLOW-703

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

• https://issues.apache.org/jira/browse/AIRFLOW-703

Per Apache guidelines you need to create a Jira issue.

Testing Done:
Expanded on a test case which was checking only a subset of this bug

• Unittests are required, if you do not include new unit tests please
  specify why you think this is not required. We like to improve our
  coverage so a non existing test is even a better reason to include one.

Reminders for contributors (REQUIRED!):

• Your PR's title must reference an issue on
  Airflow's JIRA.
  For example, a PR called "[AIRFLOW-1] My Amazing PR" would close JIRA
  issue Improving the search functionality in the graph view #1. Please open a new issue if required!

• For all PRs with UI changes, you must provide screenshots. If the UI changes are not obvious, either annotate the images or provide before/after screenshots.

• Please squash your commits when possible and follow the How to write a good git commit message.
  Summarized as follows:

  1. Separate subject from body with a blank line
  2. Limit the subject line to 50 characters
  3. Do not end the subject line with a period
  4. Use the imperative mood in the subject line (add, not adding)
  5. Wrap the body at 72 characters
  6. Use the body to explain what and why vs. how

[codecov-io]

Current coverage is 66.59% (diff: 100%)

Merging #1951 into master will increase coverage by <.01%

@@ master #1951 diff @@
==========================================
Files 135 135 Lines 10201 10201 Methods 0 0 Messages 0 0 Branches 0 0 ==========================================
+ Hits 6792 6793 +1 + Misses 3409 3408 -1 
Partials 0 0 

Powered by Codecov. Last update e8f91c6...fd8c52f

[jlowin]

@blrnw3 the fix (moving clear_xcom_data()) looks great to me.

Are we sure that your first test is actually testing the right thing? Your comment says it won't clear because the other TI is running, but I think that TI has already ended and it's not clearing because of the mark_success flag. Maybe just a clarification.

[blrnw3]

Thanks @jlowin. I've clarified the comment.

It's a little tricky to test the exact situation I found, because it relies to some extent on a race condition. The test I wrote simulates that by ignoring dependencies and marking success, which I think is a valid test because that is also a potential real-world use case, and more-easily testable.

[jlowin]

Ok, I think that's reasonable -- just wanted to double check. Once Travis clears I will merge. Thanks!

[jlowin]

Looks like I just exposed a PR-Tool bug that added [AIRFLOW-1] to the commit title. I'll look in to why!

[jlowin]

Oh, it's because the boilerplate PR template has [AIRFLOW-1] in it. Interesting.

[blrnw3]

ah my mistake 😅

[jlowin]

No worries. Thanks for the PR!

[blrnw3]

It was a pleasure