s3_request: fix broken requests_in_flight counter #216
Conversation
|
Cancel tests seem to sporadically fail after this change. Im guessing this is due to removal of s_s3_client_schedule_process_work_synced call, which results in cancel not wrapping up pending requests in some cases. |
@DmitriyMusatkin - that is correct. My suggested implementation in #214 was short-sighted and buggy, since it only decreased the The condition was not handled in I will update the notes in #214 and submit an update to this PR after some more testing. |
…mpletion (awslabs#217) **Issue**: If a function was called on a meta-request after it completed, this would lead to a crash (this affected `aws_s3_meta_request_increment_read_window()`, `aws_s3_meta_request_cancel()`, and `aws_s3_meta_request_pause()`) Since these functions might be called from different threads than the thread that completes the meta-request, it would be nearly impossible for a customer to avoid these crashes. **Description of changes:** The reason for the crash was that the `meta_request->client` pointer would get cleared when the meta-request completed. To fix this bug, we stop clearing this pointer early, and keep`meta_request->client` pointer valid for the lifetime of the meta-request. We evaluated doing something much more complex (checking state and toying with reference counts before scheduling work on the client) but it seemed finicky and error-prone.
Issue #, if available: - CRT S3 client doesn’t allow to config the checksum location, it will be always set to the trailer and the current API doesn't support the decision of checksum location - CRT S3 client doesn't support sse-c with flexible checksum, refer to here Description of changes: - Changed the API for flexible checksum - Fixed the copy of the header for sse-c
- Support list of checksum algorithms to validate - Changed the behavior that: - When all the parts of the object checksum have been validated, we still tell user that we did not validate the checksum - When some part of the object checksum validation failed, we tell user checksum validation failed, but we did not validate the checksum. TODO: - Tests for checksum validation failed. <- We don't have mock server, we cannot control that...
This fixes a bug in computing the exact requests-in-flight.
These were not decremented in some case, due to the
clientfield beingNULLwhen the request was destroyed.Instead of decrementing
num_requests_in_flightand scheduling follow-on work when the request is destroyed,perform this task when calling
aws_s3_meta_request_finished_request(which signals that the request is doneand will not be retried), using a wrapper function.
Issue #, if available: Resolves #214
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.