HD-3340 drain stream when receiving an error on a PUT

[benoit-a]

No description provided.

[bert-e]

Hello benoit-a,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

[bert-e]

Waiting for approval

The following approvals are needed before I can proceed with the merge:

• the author

• one peer

[williamlardier]

Changes LGTM but given that it's a bugfix ficket, I would prefer that we also create some new unit tests to ensure that the fix is enough, and that we don't reproduce this problem again in the future

[vietnscality]

Changes LGTM but given that it's a bugfix ficket, I would prefer that we also create some new unit tests to ensure that the fix is enough, and that we don't reproduce this problem again in the future

@williamlardier Maybe you did not see this comment https://github.com/scality/hdclient/pull/659/files#r1356780040

[williamlardier]

Hello @vietnscality , yes I saw the comment, mine is more about adding some unit tests to reproduce the initial problem and making sure we won't have any regression for this behaviour in the future

[vietnscality]

Hello @vietnscality , yes I saw the comment, mine is more about adding some unit tests to reproduce the initial problem and making sure we won't have any regression for this behaviour in the future

Yes, i agree, it is a good point. The pb is AWAIU since the PR does not fix the issue, having a test will fail the CI. Ideally the unit test should go together with the fix. Or maybe I missed something?

[williamlardier]

@vietnscality we can .skip it if needed, till we have a proper fix; but what I had in mind was to test the current code, i.e. consider that we resume the readable stream even if err is not defined (and update it later with the real fix), make sure there is no side effects upstream (in cloudserver) with the current code, and don't add too much technical debt

It's not a blocking comment anyway 😄

[vietnscality]

@vietnscality we can .skip it if needed, till we have a proper fix; but what I had in mind was to test the current code, i.e. consider that we resume the readable stream even if err is not defined (and update it later with the real fix), make sure there is no side effects upstream (in cloudserver) with the current code, and don't add too much technical debt

It's not a blocking comment anyway 😄

OK, fair enough, I'll let the author @benoit-a decide what is appropriate for him

[benoit-a]

OK, fair enough, I'll let the author @benoit-a decide what is appropriate for him

Good point for the test part, I will how I can do to detect the behaviour on a unit test

Regarding the rest of the discussion, I think there is confusion ; to me, the real bug is here, hdclient MUST close the session and for that, it has to drain the stream. It should do that every time there is a response

The changes in hdcontroller are merely "good practices" that could have helped slightly but the client part (cloudserver) would still exhibit issues (possibly accumulating sockets in CLOSE WAIT or file descriptor)

[williamlardier]

LGTM!

[benoit-a]

/approve

[bert-e]

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

• ✔️ development/1.1

The following branches will NOT be impacted:

• development/1.0

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

• Any commit you add on the source branch will trigger a new cycle after the
  current queue is merged.
• Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

[bert-e]

I have successfully merged the changeset of this pull request
into targetted development branches:

• ✔️ development/1.1

The following branches have NOT changed:

• development/1.0

Please check the status of the associated issue HD-3340.

Goodbye benoit-a.