Opened 4 years ago

Closed 3 years ago

#16540 closed defect (invalid)

Close the the updater's shared HttpUrlConnection socket if an exception occurs.

Reported by: leeroy Owned by: karsten
Priority: Medium Milestone:
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: #16426 Points:
Reviewer: Sponsor:

Description

When Onionoo's updater uses a HttpUrlConnection it doesn't attempt to close the socket after an exception. An exception may occur anytime during or after the first fetchRemoteDirectory of DescriptorSource. The problem is this can leave the HttpUrlConnection's shared underlying network connection in a state which blocks further requests. This has the effect of preventing reads as early as during the first fetchRemoteFiles. Onionoo's updater will then block until the problem HttpUrlConnection is handled.

To reproduce. Close the connection remotely during fetchRemoteDirectory.

Child Tickets

Attachments (5)

Change History (20)

comment:1 Changed 4 years ago by leeroy

Summary: Close the the updater's shared HttpUrlConnection socket if an exceptions occurs.Close the the updater's shared HttpUrlConnection socket if an exception occurs.

comment:2 Changed 4 years ago by karsten

Component: Onionoometrics-lib
Keywords: onionoo httpurlconnection descriptordownloader could not fetch or parse store removed
Owner: set to karsten

Good point. But would you want to fix that same problem in metrics-lib and change Onionoo to use that code instead? Optimistically changing the component to metrics-lib for the first part. Once that's done, let's create a new ticket for the Onionoo change. Thanks!

comment:3 Changed 4 years ago by leeroy

Sounds like a fine plan. I see just the three spots where blocking can occur. Unfortunately HttpURLConnection isn't AutoCloseable so it cannot be used in a try-with-resources.

comment:4 Changed 4 years ago by leeroy

Status: newneeds_review

comment:5 Changed 4 years ago by karsten

Component: metrics-libOnionoo
Status: needs_reviewnew

Looks good, merged with trivial code-style tweaks. Changing component back to Onionoo. Thanks!

comment:6 Changed 4 years ago by leeroy

I took a look at changing Onionoo to use metrics-lib. There are a couple gotchas. DescriptorSource uses some data from DescriptorCollector when writing in getStatsString. As a first attempt I made the DescriptorDownloader inherit from DescriptorCollector. This required the access changes described in metric-lib's DescriptorCollectorImpl. Here are the changes.

comment:7 Changed 4 years ago by leeroy

Status: newneeds_review

comment:8 Changed 4 years ago by karsten

Status: needs_reviewneeds_revision

Ah, I'd rather want to give up on the stats and instead use metrics-lib's interface. Extending metrics-lib's impl classes is something we shouldn't start doing.

To give you some more context, the metrics-lib code to fetch descriptors from CollecTor was written first and then re-implemented slightly more generally in metrics-lib. The earlier Onionoo code is not required anymore. In theory, the Onionoo change here should remove many lines and only add very few new ones. If this means leaving out stats log messages for now, I'm fine with that.

In the future we could even have metrics-lib log these stats itself to a logger that Onionoo includes in its log files. (That would be another ticket.)

Hope that makes sense!

comment:9 Changed 4 years ago by leeroy

It does.

That first revision was an attempt to avoid changing metrics-lib. Clearly it's a messy attempt. It sounds like you agree so I went back and improved the utility of DescriptorCollectorImpl? for a general case (realized, for example, using Onionoo). This next version uses the improved utility to reduce code clutter.

Last edited 4 years ago by leeroy (previous) (diff)

comment:10 Changed 4 years ago by leeroy

Status: needs_revisionneeds_review

comment:11 Changed 4 years ago by leeroy

Don't test those new attachment yet. It generates some ERROR log entries during descriptor read (after download). It's due to not being able to parse. I'm looking into the error.

comment:12 Changed 4 years ago by leeroy

The error log entries appear to have been caused by line 196 of DescriptorCollectorImpl. It is now fixed.

Changed 4 years ago by leeroy

Revised fix.

comment:13 Changed 4 years ago by leeroy

A potential bug may happen if DescriptorCollectorImpl is given an empty string instead of a remote directory. If this happens the fetch will happen and return the directory contents. This probably isn't what you want to happen so the recently updated patch includes everything else plus a fix for this event.

Version 0, edited 4 years ago by leeroy (next)

Changed 4 years ago by leeroy

Revised Onionoo fix.

comment:14 Changed 4 years ago by leeroy

Parent ID: #16426

comment:15 Changed 3 years ago by iwakeh

Resolution: invalid
Severity: Normal
Status: needs_reviewclosed

This is obsolete metrics-lib uses DescriptorIndexCollector as default since Oct 2016.
The use of HttpURLConnection should be avoided (there is a ticket about this, too).

Closing.

Note: See TracTickets for help on using tickets.