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 jclouds/apis/swift retryOnRenew #1027

Closed
wants to merge 1 commit into
base: 1.9.x
from

Conversation

Projects
None yet
4 participants
@andreaturli
Contributor

andreaturli commented Oct 26, 2016

/cc @nacx

@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 26, 2016

Contributor

I think you are right!

Contributor

andreaturli commented Oct 26, 2016

I think you are right!

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Oct 26, 2016

Member

BTW, there are a couple more RetryOnRenew classes that do the same thing. Mind removing the finally block from all them too?

Member

nacx commented Oct 26, 2016

BTW, there are a couple more RetryOnRenew classes that do the same thing. Mind removing the finally block from all them too?

@gaul

This comment has been minimized.

Show comment
Hide comment
@gaul

gaul Oct 26, 2016

Member

Easier to review when ignoring whitespace:

https://github.com/jclouds/jclouds/pull/1027/files?w=1

@andreaturli Can you expand on the problem this solves?
@zack-shoylev Can you review this since you wrote RetryOnRenew?

Member

gaul commented Oct 26, 2016

Easier to review when ignoring whitespace:

https://github.com/jclouds/jclouds/pull/1027/files?w=1

@andreaturli Can you expand on the problem this solves?
@zack-shoylev Can you review this since you wrote RetryOnRenew?

@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 26, 2016

Contributor

@andrewgaul retriers in general shouldn't close the streams if they are not reading the payload, as https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L144 will close it anyway.

Contributor

andreaturli commented Oct 26, 2016

@andrewgaul retriers in general shouldn't close the streams if they are not reading the payload, as https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L144 will close it anyway.

@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 26, 2016

Contributor

rebuild please

Contributor

andreaturli commented Oct 26, 2016

rebuild please

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Oct 26, 2016

Member

Can you expand on the problem this solves?

Upon failed requests (response code > 3xx), the BaseHttpCommandExecutorService decides if the request should be retried or not by asking the configured retry handler. If the retry handler says no, then the request is considered failed and passed to the error handler so a proper exception is propagated. Otherwise, a new request with the same data is issued.

Before this commit, error handlers and retry handlers were responsible of closing the payload, which was not good, as both implementations needed to be aware of each other. That commit just makes the caller release the payload when needed, so in general retry and error handlers no longer need to release it. The shouldContinue method in the BaseHttpCommandExecutorService will always release the payload when needed.

The only thing to take into account is that retry handlers that need to read the payload to determine if the request should be retried need to buffer the payload or do whatever it takes (if the payload is not replayable) to make sure the error handler will be able to read the payload again from the beginning, in case the response is considered failed.

The problem this issue fixes is that in some circumstances, the error handler got a closed stream, since the retry handler closed it (this only appeared when the jclouds wire logs were disabled, since payloads are saved when using wire logs).

In summary, retry and error handlers should not release the payload. If retry handlers need to read it, they need to make sure the response is updated with a payload that can be read again from the beginning.

Member

nacx commented Oct 26, 2016

Can you expand on the problem this solves?

Upon failed requests (response code > 3xx), the BaseHttpCommandExecutorService decides if the request should be retried or not by asking the configured retry handler. If the retry handler says no, then the request is considered failed and passed to the error handler so a proper exception is propagated. Otherwise, a new request with the same data is issued.

Before this commit, error handlers and retry handlers were responsible of closing the payload, which was not good, as both implementations needed to be aware of each other. That commit just makes the caller release the payload when needed, so in general retry and error handlers no longer need to release it. The shouldContinue method in the BaseHttpCommandExecutorService will always release the payload when needed.

The only thing to take into account is that retry handlers that need to read the payload to determine if the request should be retried need to buffer the payload or do whatever it takes (if the payload is not replayable) to make sure the error handler will be able to read the payload again from the beginning, in case the response is considered failed.

The problem this issue fixes is that in some circumstances, the error handler got a closed stream, since the retry handler closed it (this only appeared when the jclouds wire logs were disabled, since payloads are saved when using wire logs).

In summary, retry and error handlers should not release the payload. If retry handlers need to read it, they need to make sure the response is updated with a payload that can be read again from the beginning.

@zack-shoylev

This comment has been minimized.

Show comment
Hide comment
@zack-shoylev
Contributor

zack-shoylev commented Oct 26, 2016

@zack-shoylev

This comment has been minimized.

Show comment
Hide comment
@zack-shoylev

zack-shoylev Oct 26, 2016

Contributor

test408ShouldRetry for 2.0 should also be updated, it seems?

Contributor

zack-shoylev commented Oct 26, 2016

test408ShouldRetry for 2.0 should also be updated, it seems?

@zack-shoylev

This comment has been minimized.

Show comment
Hide comment
@zack-shoylev

zack-shoylev Oct 26, 2016

Contributor

Good change! Thanks, @andreaturli

Contributor

zack-shoylev commented Oct 26, 2016

Good change! Thanks, @andreaturli

Fix retryOnRenew classes
These classes should not close the release the payload as they are not
reading it.

- fix swift
- fix openstack-swift v1 and v2
- fix RetryOnRenewTest for v1 and v2
@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 27, 2016

Contributor

thanks @zack-shoylev, fixed v2 as well!

Contributor

andreaturli commented Oct 27, 2016

thanks @zack-shoylev, fixed v2 as well!

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Oct 27, 2016

Member

@zack-shoylev Could you give a try to this PR and check if this fixes JCLOUDS-1176?

Member

nacx commented Oct 27, 2016

@zack-shoylev Could you give a try to this PR and check if this fixes JCLOUDS-1176?

@zack-shoylev

This comment has been minimized.

Show comment
Hide comment
@zack-shoylev

zack-shoylev Oct 27, 2016

Contributor

@nacx I have had some issues reproducing 1176. But I think this is alright to merge even if it does not fix it right away (but I strongly suspect it will).

Contributor

zack-shoylev commented Oct 27, 2016

@nacx I have had some issues reproducing 1176. But I think this is alright to merge even if it does not fix it right away (but I strongly suspect it will).

@nacx

nacx approved these changes Oct 28, 2016

lgtm!

@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 28, 2016

Contributor

merged at 1.9.x

@nacx @zack-shoylev shall we port that to master as well?

Contributor

andreaturli commented Oct 28, 2016

merged at 1.9.x

@nacx @zack-shoylev shall we port that to master as well?

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Oct 28, 2016

Member

Absolutely. There are no plans to release 1.9.x, and it's been a while since we stopped backporting fixes.

Member

nacx commented Oct 28, 2016

Absolutely. There are no plans to release 1.9.x, and it's been a while since we stopped backporting fixes.

@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 28, 2016

Contributor

ok cool. I think I will volunteer for a new 1.9.x release :)

Contributor

andreaturli commented Oct 28, 2016

ok cool. I think I will volunteer for a new 1.9.x release :)

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Oct 28, 2016

Member

ok cool. I think I will volunteer for a new 1.9.x release :)

Perfect! :) There is no reason to not release if there are volunteers willing to do so!

Member

nacx commented Oct 28, 2016

ok cool. I think I will volunteer for a new 1.9.x release :)

Perfect! :) There is no reason to not release if there are volunteers willing to do so!

@andreaturli

This comment has been minimized.

Show comment
Hide comment
@andreaturli

andreaturli Oct 31, 2016

Contributor

Merged at master and 1.9.x

Contributor

andreaturli commented Oct 31, 2016

Merged at master and 1.9.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment