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

[FIX] website_sale: prevent race condition on tour #44113

Closed

Conversation

rdeodoo
Copy link
Contributor

@rdeodoo rdeodoo commented Jan 28, 2020

As this race condition seems not possible to be reproduced in local, all this
is an assumption based on the logs.

After the checkout, the user is redirected to /shop/confirmation where there
is RPCs fired ever seconds to fetch the payment status
(/shop/payment/get_status).

As the test was considered as finished on that page, sometimes the RPC would
still be processed in python while the tour was considered done and killed,
thus also the cookies, session & co.
From that point, the python would crash when accessing the session.
In the logs, the python crash in the RPC call occurs after the test is done.

@rdeodoo
Copy link
Contributor Author

rdeodoo commented Jan 28, 2020

@d-fence I wasn't able to reproduce it, but based on the logs I think it makes sense, could you check the commit message and gimme your feedback?

@rdeodoo
Copy link
Contributor Author

rdeodoo commented Jan 28, 2020

cc @JKE-be As you already have worked on this one, maybe you have some other input on this?

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jan 28, 2020
Copy link
Contributor

@d-fence d-fence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do you need a runbot multi-build on your branch ?

addons/website_sale/static/src/js/website_sale_tour_buy.js Outdated Show resolved Hide resolved
@C3POdoo C3POdoo added the RD research & development, internal work label Jan 28, 2020
@rdeodoo rdeodoo force-pushed the 12.0-fix-attempt-runbot-test-fail-de branch from 5b160f5 to e6eb468 Compare January 28, 2020 16:29
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Jan 28, 2020
@rdeodoo rdeodoo force-pushed the 12.0-fix-attempt-runbot-test-fail-de branch from e6eb468 to bfb5a79 Compare January 28, 2020 16:31
@rdeodoo
Copy link
Contributor Author

rdeodoo commented Jan 28, 2020

JKE enabled multi-build, I'll rebuild 10-20 times to ensure 150-300 builds are green (or not).

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jan 28, 2020
@rdeodoo
Copy link
Contributor Author

rdeodoo commented Jan 29, 2020

@d-fence By no mean I am 100% sure this is fixing the issue. But in theory it should.
I rebuilt it 10 times and still no red build, but as it happen only once a day, that's probably 1/5000 builds so..

@d-fence
Copy link
Contributor

d-fence commented Jan 29, 2020

@d-fence By no mean I am 100% sure this is fixing the issue. But in theory it should.
I rebuilt it 10 times and still no red build, but as it happen only once a day, that's probably 1/5000 builds so..

I agree, I just put back the "Split" config and I will rplus

@d-fence
Copy link
Contributor

d-fence commented Jan 29, 2020

I wait the last split to finish and rebuild with the Split config

@JKE-be
Copy link
Contributor

JKE-be commented Jan 29, 2020

@rdeodoo can we test with a light page instead of /shop

@d-fence
Copy link
Contributor

d-fence commented Jan 29, 2020

Ok, I re-enabled multi-build to check with a lighter page

@JKE-be
Copy link
Contributor

JKE-be commented Jan 29, 2020

If it works with shop it will works with another page.
Don’t Need to retest imo...
just don’t slow test to don’t test anything more:)

@d-fence
Copy link
Contributor

d-fence commented Jan 29, 2020

ok, back to Split then

@rdeodoo
Copy link
Contributor Author

rdeodoo commented Jan 29, 2020

@JKE-be Yes I know it would be better, I thought about it. I used homepage at first but no way to know we landed on homepage. I just need a page with an unique element on it.
I could use contactus or aboutus, I will change.

As this race condition seems not possible to be reproduced in local, all this
is an assumption based on the logs.

After the checkout, the user is redirected to `/shop/confirmation` where there
is RPCs fired ever seconds to fetch the payment status
(`/shop/payment/get_status`).

As the test was considered as finished on that page, sometimes the RPC would
still be processed in python while the tour was considered done and killed,
thus also the cookies, session & co.
From that point, the python would crash when accessing the session.
In the logs, the python crash in the RPC call occurs after the test is done.
@rdeodoo rdeodoo force-pushed the 12.0-fix-attempt-runbot-test-fail-de branch from bfb5a79 to 7764246 Compare January 29, 2020 13:06
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Jan 29, 2020
@rdeodoo
Copy link
Contributor Author

rdeodoo commented Jan 29, 2020

Chang done, this can be r+?

@JKE-be
Copy link
Contributor

JKE-be commented Jan 29, 2020

@robodoo r+

Thx

robodoo pushed a commit that referenced this pull request Jan 29, 2020
As this race condition seems not possible to be reproduced in local, all this
is an assumption based on the logs.

After the checkout, the user is redirected to `/shop/confirmation` where there
is RPCs fired ever seconds to fetch the payment status
(`/shop/payment/get_status`).

As the test was considered as finished on that page, sometimes the RPC would
still be processed in python while the tour was considered done and killed,
thus also the cookies, session & co.
From that point, the python would crash when accessing the session.
In the logs, the python crash in the RPC call occurs after the test is done.

closes #44113

Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
@rdeodoo
Copy link
Contributor Author

rdeodoo commented Feb 3, 2020

If I am correct, this one did not appear since it was fixed 👍

@d-fence
Copy link
Contributor

d-fence commented Feb 4, 2020

@rdeodoo True. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants