Opened 3 years ago

Closed 23 months ago

#20515 closed enhancement (fixed)

CollecTor's relaydescs module should avoid httpurlconnection

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.2.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: metrics-help
Cc: iwakeh Actual Points:
Parent ID: #8799 Points:
Reviewer: Sponsor:

Description

See #20323:

RelayDescriptorDownloader should use

      try (InputStream is = new URL(baseUrl + "/" + filepathname)
          .openStream()) {
        Files.copy(is, tempDestinationFile.toPath());
...

Child Tickets

Change History (6)

comment:1 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.2.0

comment:2 Changed 2 years ago by iwakeh

Cc: iwakeh added

Adding myself to cc to make trac mail updates.

comment:3 Changed 23 months ago by karsten

See #22602 for a stack trace of this bug or a closely related bug.

comment:4 Changed 23 months ago by iwakeh

Owner: set to iwakeh
Status: newaccepted

comment:5 Changed 23 months ago by iwakeh

Status: acceptedneeds_review

Simply switching to file copy would mean a major change here as the current download approach reads the data to memory for verifying and after that writes to file(s).
A toy example of an http-server that accepts connections, but never writes the response code yielded the stack-trace provided in #22602. The easiest remedy is setting a read-timeout.
(A quick inspection of the code that gets used when using Files.copy indicates that a timeout seems to be set there, too.)

Open points:

  • the current read timeout is arbitrarily set to 5000ms. How to find the best value?
  • Should a connect timeout be set, too? There is no bug report that indicates necessity for this timeout type.

Please review this change.

PS: There are no other occurrences of HttpUrlConnection in this module.

Last edited 23 months ago by iwakeh (previous) (diff)

comment:6 Changed 23 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Patch looks good and is now merged to master! I believe 5000ms is as good as a first guess as any other value. We'll find out if it's too low or high over time, and we'll find out if we should have set a connect timeout as well. I'd say let's start with this, it's better than what we did before. Closing as fixed. Thanks!

Note: See TracTickets for help on using tickets.