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.

Sign up for GitHub

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

Jump to bottom

Incomplete responses lead to failure-loop #59

Open
Joacchim opened this issue Nov 5, 2014 · 3 comments
Open

Incomplete responses lead to failure-loop #59

Joacchim opened this issue Nov 5, 2014 · 3 comments
Assignees
@scalityQAProd
Labels
bug

Comments

@Joacchim
Copy link
Contributor

Joacchim commented Nov 5, 2014

With the fix for #54 , a new bug was introduced:
When the reponse cannot be read in its entirety (whatever the reason, but the bug was shown thanks to an error in the DELETE op of the playground server), the sock_send_receive function tries to re-send the request, which was partially overwritten while reading the response.

This means that either:

  • We must "receive" in a different temporary buffer
  • We must not retry to transmit the request

Also:

  • The retry must be limited to a given number of times (otherwise the fail loop won't ever be stopped)
  • The status returned when the fail-loop is interrupted or when a partial response was received should reflect the fact that the response isn't complete (returning a negative status ? Which errno code? another method ?)
@Joacchim Joacchim self-assigned this Nov 5, 2014
@Joacchim
Copy link
Contributor Author

Joacchim commented Nov 6, 2014

Since a few retries mechanisms are implemented on top of those functions, I'm guessing that it's not that useful to have it within the function (or we should completely do it within the function, and not on top of it at all ?)

So my plan is to remove the retry when some data could be read (maybe even in all cases, I'm not sure ?), AND to return an errno code (probably EIO). What do you think about it ?

@Joacchim Joacchim mentioned this issue Nov 6, 2014
Open
@Joacchim
Copy link
Contributor Author

Joacchim commented Nov 6, 2014

Discussed with vianney, should be ok with #60 as an addition.

@Joacchim Joacchim added the bug label Nov 6, 2014
Joacchim added a commit that referenced this issue Nov 13, 2014
@Joacchim
Read HTTP response in temp buffer: Fix issue #59
22b1aa7 
Now reads of the response is done in temp buffer to avoid mixing request and
response
Joacchim added a commit that referenced this issue Nov 13, 2014
@Joacchim
First part of the work for #60 (+ finalize #59)
8ca8fd0 
 - Change the way to manage disconnects while sending a request or receiving a
response.
 - Now Only retry once in case of disconnect

The only unadressed part of #59 was the return status of the operation. For
simplicity, either the response could be fully retrieved, and we return the
length of the reponse; or it couldn't and we return the last error code
(whichever it is).

Thus we will consider that this patch completes the fix to issue #59.
@Joacchim Joacchim assigned scalityQAProd and unassigned Joacchim Nov 13, 2014
@Joacchim
Copy link
Contributor Author

Cannot reproduce it anymore with the playground server, I personally consider this as fixed; although it might be better to test a bit more.
Previous failing scenario:

  • Send a request through the driver
  • response partially sent
  • the CDMI server closes the connection, provoking a retry on the SRB side
  • the buffer of the request/responses now is partially erased by the response
  • The CDMI server returns a 400 Bad Request because the request has been corrupted when reading the beginning of the response.

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

@scalityQAProd scalityQAProd

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Joacchim @scalityQAProd