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

Retry on ConflictError is broken #413

Closed
icemac opened this issue Dec 10, 2018 · 8 comments
Closed

Retry on ConflictError is broken #413

icemac opened this issue Dec 10, 2018 · 8 comments
Assignees
Labels
bug
Milestone

Comments

@icemac
Copy link
Member

@icemac icemac commented Dec 10, 2018

Retrying of requests in case of an ConflictError is broken on multiple levels:

  • HTTPRequest.retry_max_count = 0 meaning no retries by default. This default is not not changed when setting max_conflict_retries in zope.conf See [1] below.
  • If an exception view for Exception is reqistered it has to reraise ConflictException otherwise it will not be handled by the retry mechanism. See [2] below.
  • If a request is actually retried the newly created event gets closed before processing it. See [3] below and #414.
  • ConflictError is a subclass of TransientError, the code using both can be simplified. See [4] below.
  • HTTPRequest.supports_retry() sleeps when called. See [5] below. Moved to #474.

[1] The default used to be 3 in Zope 2.13 but was changed in 740eb60 to 0. In 40e5f99 the ability to change the default was added but it sets the value on ZPublisher.HTTPRequest (the module) instead of ZPublisher.HTTPRequest.HTTPRequest (the class), so it does not have the expected effect.

[2] This seems unexpected to me. Maybe ConflictError should be handled separately like it is already done for Unauthorized.

[3] The request.close() in the finally block (see WSGIPublisher.publish_module) closes the newly created request, so it is empty during the retry.

[4] Since zopefoundation/ZODB@364b2a2 a ZODB ConflictError is a subclass of transaction's TransientError. The commit is part of ZODB 3.10+.

[5] I'd not expect that a method called supports_retry does the actual sleeping. It is called three times when a ConflictError happens.

@icemac icemac added the bug label Dec 10, 2018
@icemac icemac added this to the 4.0 final milestone Dec 10, 2018
@icemac icemac added this to To do in Zope 4 final release via automation Dec 10, 2018
icemac added a commit that referenced this issue Dec 10, 2018
icemac added a commit that referenced this issue Dec 14, 2018
@dataflake
Copy link
Member

@dataflake dataflake commented Feb 2, 2019

ba72a30 fixes the configuration setting, 40e5f99 was clearly faulty.

@dataflake
Copy link
Member

@dataflake dataflake commented Feb 2, 2019

If an exception view for Exception is reqistered it has to reraise ConflictException otherwise it will not be handled by the retry mechanism. See [2] below.

Yes, that makes a lot of sense. I'm guessing you're referring to

if not (exc_view_created or isinstance(exc, Unauthorized)):
and just adding ConflictError to the condition, right?

@dataflake
Copy link
Member

@dataflake dataflake commented Feb 2, 2019

ConflictError is a subclass of TransientError, the code using both can be simplified. See [4] below.

Fixed in 4dc1700 - there was just a single place in the whole code base that referenced both error types at the same time.

@dataflake dataflake self-assigned this Feb 2, 2019
@icemac
Copy link
Member Author

@icemac icemac commented Feb 6, 2019

@dataflake wrote:
[…]

Zope/src/ZPublisher/WSGIPublisher.py Line 170 in [ba72a30]

and just adding ConflictError to the condition, right?

Yes, I think this should be enough.

@dataflake
Copy link
Member

@dataflake dataflake commented Feb 7, 2019

if not (exc_view_created or isinstance(exc, Unauthorized)):

@icemac Maybe its too late and I am having a brain fart, but if an exception view for Exception is registered this condition will never be fulfilled (exc_view_created is True at that moment) and the type of exception is not even checked anymore. So the view will also swallow Unauthorized.

@icemac
Copy link
Member Author

@icemac icemac commented Feb 7, 2019

@dataflake You are right: creating exec_view currently prevents reraising of the exception.
Swallowing Unauthorized might be okay as the exception view might render a login page.

@dataflake
Copy link
Member

@dataflake dataflake commented Feb 8, 2019

[5] I'd not expect that a method called supports_retry does the actual sleeping. It is called three times when a ConflictError happens.

If you don't have the time.sleep centrally on the HTTPRequest every type of publisher would have to implement that delay itself when a conflict is encountered. With Python 2.7 we still have two publishers, WSGI and ZServer. A better moment to move that delay is when Python 2 support is discontinued and there's just one publisher left.

@icemac
Copy link
Member Author

@icemac icemac commented Feb 8, 2019

Regarding HTTPRequest.supports_retry(): My naive expectation would have been that there is a method for checking and one for doing the actual wait. @dataflake You are right we can postpone this to Zope 5. I have created #474 for this issue.

@icemac icemac closed this in #473 Feb 9, 2019
Zope 4 final release automation moved this from To do to Done Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants