twitter/finagle

Problem

We're on a version of jackson from October 2016, and it has a [CVE][0] against it.
Furthermore, there are bugfixes and feature improvements in new jackson, which
we would like to use.

Solution

Upgrade to the latest and greatest version of jackson.

[0]: https://nvd.nist.gov/vuln/detail/CVE-2017-7525

JIRA Issues: CSL-6599

Differential Revision: https://phabricator.twitter.biz/D204133

…tFactory to the close method

Problem

ServiceClosedException in LazyEndpointFactory tells you who tried to use the
endpoint factory after it was closed, but not who closed it.

Solution

Create ServiceClosedException only once, when it's closed, and remember it
thereafter.  Also provide a useful exception message to SCE.

JIRA Issues: CSL-6816

Differential Revision: https://phabricator.twitter.biz/D200423

Problem

HeaderSensitivityDetector is hooked up properly for h2, but not for h2c or
prior knowledge.

Solution

Hook up the header sensitivity detector unconditionally.

JIRA Issues: CSL-6657

Differential Revision: https://phabricator.twitter.biz/D195258

Problem

It's not easy to mark everything as sensitive, which can be useful for disabling
caching in the header table.

Solution

Provide a configuration option that marks everything as sensitive.

Differential Revision: https://phabricator.twitter.biz/D191775

Problem

When a BackupRequestFilter raises on a dispatch that has already been sent over
the wire, mux sends a Tdiscarded message so that the remote peer knows that the
work is no longer needed.  When the server receives the message, it interrupts
the dispatch on its end.  However, it loses the `Ignorable` status when it goes
over the network.

Solution

We should inspect the `why` section of the Tdiscarded to see if it's from
BackupRequestFilter, and if it is, then we should mark it as `Ignorable`.

Result

Stats and failure accrual on receivers don't get polluted by BackupRequestFilter
interruptions from the receiver's callers.

JIRA Issues: CSL-6516

Differential Revision: https://phabricator.twitter.biz/D183456

Problem

When we profile h2 servers, we see contention in MetricsBucketedHistogram#add.

Solution

Change BucketedHistogram to be memory safe and remove intrinsic locking from
MetricsBucketedHistogram.  I tried ReentrantReadWriteLock in BucketedHistogram,
but it was too slow.  I tried several cheap read-write lock implementations
(lock on updating an integer and spin, atomic integer and spin, semaphore, StampedLock)
before finally settling on AQS, which is very fast.  I also started using an
AtomicIntegerArray so that racing in `add` would be safe.  However, converting
`num` and `total` to AtomicIntegers also lost us some speed.

JIRA Issues: CSL-5626

Differential Revision: https://phabricator.twitter.biz/D173413

Problem

We use binary search to find which bucket to use with our histograms, which
incurs a bunch of comparison operations.

Solution

Since our bucketing scheme is exponential, we can take the logarithm to look
up which bucket to use.

Result

Old:
Result "com.twitter.finagle.stats.StatsReceiverBenchmark.addMetrics":
  305.403 ±(99.9%) 21.344 ns/op [Average]
  (min, avg, max) = (279.395, 305.403, 320.314), stdev = 14.118
  CI (99.9%): [284.059, 326.747] (assumes normal distribution)

New:

Result "com.twitter.finagle.stats.StatsReceiverBenchmark.addMetrics":
  237.653 ±(99.9%) 16.106 ns/op [Average]
  (min, avg, max) = (225.779, 237.653, 259.360), stdev = 10.653
  CI (99.9%): [221.546, 253.759] (assumes normal distribution)

~20% faster

JIRA Issues: CSL-6430

Differential Revision: https://phabricator.twitter.biz/D173981

Problem

We have to register and deregister a closable in Closables every time we receive
a new request and then respond to it in h2.  High throughput servers have seen
lock contention from this.

Solution

Use a ReadWriteLock instead of an intrinsic lock, and switch to using a CHM
instead of a HashSet.

JIRA Issues: CSL-5971

Differential Revision: https://phabricator.twitter.biz/D171480

Problem

Various tests were marked as flaky, some correctly, some incorrectly.

Solution

Unflaky everything!  After running this test 1000 times there were 0 failures.

JIRA Issues: CSL-5888

Differential Revision: https://phabricator.twitter.biz/D171344

Problem

StandardToggleMap has an underlying mutable map, but it's difficult to access.

Solution

Make StandardToggleMap a `ToggleMap.Mutable` that proxies the mutating method
to the underlying mutable map.

JIRA Issues: CSL-6348

Differential Revision: https://phabricator.twitter.biz/D167046

Summary: Problem

ToggleMap and Toggle found a bunch of java compatibility landmines.

1. traits that extend abstract classes don't propagate their inheritance
to java.
2. methods that return generics with a primitive subtype are compiled
as if they return an object subtype.  jdk8 disallowed casting from Gen<Object>
to Gen<SpecificType>.
3. PartialFunction[A, Boolean]#apply returns an Object instead of a boolean.

Solution

Add helpers to ToggleMap and Toggle to return a better type in java, and change
everything that extends ToggleMap to be an abstract class.  For ToggleMap.Proxy,
which is mixed in with other classes, we should instead change it to no longer
extend ToggleMap, and instead have a self-type which conforms to ToggleMap.

JIRA Issues: CSL-6325

Differential Revision: https://phabricator.twitter.biz/D164489

Summary: Problem

Although it's clear to scala users of finagle that none of our APIs take nulls,
some java users can be confused by this.

Solution

Document our policy on nulls in the FAQ.

Differential Revision: https://phabricator.twitter.biz/D163810

Summary: Problem

The c.t.f.http.Request class has a hodge-podge of things on it, and it's often
difficult to divine what the different pieces are doing.  As an example, the
`Request#params` refer to things that don't come from the HTTP spec, URI query
parameters and the application/x-www-form-urlencoded MIME-type.

Solution

Start breaking out the non-HTTP pieces of `c.t.f.http.Message` and friends so
that in the long run we can slim down `c.t.f.http.Message` to something
reasonable.

Result

It's now easy to extract the URI parameters as a ParamMap.

JIRA Issues: CSL-6251

Differential Revision: https://phabricator.twitter.biz/D160298

Summary: Problem / Solution

Fixed a test to be consistent with the rest of the HTTP/2 tests so that we
consistently upgrade before testing.

JIRA Issues: CSL-6230

Differential Revision: https://phabricator.twitter.biz/D160789

Summary: Problem / Solution

One of our HTTP/2 end to end tests is flaky because of a race between when
we update some metrics and when we reset the metrics after the upgrade.

JIRA Issues: CSL-6248

Differential Revision: https://phabricator.twitter.biz/D159867

Summary: Problem / Solution

DynamicTimeoutTest had a race in swapping out the transport in RefTransport that
was fixed when we switched to upgrading only on messages without bodies.

JIRA Issues: CSL-5979

Differential Revision: https://phabricator.twitter.biz/D159801

Summary: Problem

If the remote server returns a header list that's too big, we just swallow the
error.  It would be better if we matched h1 behavior, which is to hand back
a TooLongMessageException.

Solution

Whenever Http2ConnectionHandler#onStreamError is called with an
HeaderListSizeException, match http/1.1 behavior by handing back a
TooLongMessageException

JIRA Issues: CSL-5810

Differential Revision: https://phabricator.twitter.biz/D154113

Summary: Problem

Sometimes ClosedChannelExceptions should be retryable.

Solution

Make them FailureFlags, which means that they can easily be marked Retryable.

Also fixed a flaky test that started failing consistently.

JIRA Issues: CSL-6113

Differential Revision: https://phabricator.twitter.biz/D144740

Summary: Problem / Solution

We renamed finagle-zipkin to finagle-zipkin-scribe, but missed
.travis.yml.

JIRA Issues: CSL-6090

Differential Revision: https://phabricator.twitter.biz/D142383

Summary: Problem

finagle-zipkin was originally named when scribe was the only transport
for zipkin, so zipkin structs over scribe was the zipkin protocol. That's no
longer the case--there are many different possible transports, and the
openzipkin folks want to remove the scribe transport.

Solution

Rename finagle-zipkin to finagle-zipkin-scribe so it's clear it's just an
implemenation, instead of the default.  This will discourage folks from adopting
it by default.

JIRA Issues: CSL-6090

Differential Revision: https://phabricator.twitter.biz/D141940

Summary: Problem / Solution

The travisci build became misconfigured after we deleted finagle-netty3-http.

JIRA Issues: CSL-6024

Differential Revision: https://phabricator.twitter.biz/D138209

Summary: Problem

We close the ChannelTransport many times on stream closure, which makes a bunch
of new exceptions.  These exceptions have to walk the stack trace, so they have
a performance impact.

Solution

1. Don't close the channel if channelInactive has been called on the channel.
2. Don't allow two closes.
3. Don't walk the stack when we make an exception when streams are closed.

JIRA Issues: CSL-5882

Differential Revision: https://phabricator.twitter.biz/D131457

Summary: Problem

Our http/2 client doesn't support push promises, but it sometimes receives them
from eager-beaver servers, and then barfs.  Barfing should never be the solution
when the remote peer is following the spec.

Solution

Indicate in the initial http/2 settings that we disable server push.

JIRA Issues: CSL-5958

Differential Revision: https://phabricator.twitter.biz/D135151

Summary: Problem

The ping memory leaks make it untenable to keep using the old toggle name.

Solution

Let's switch to a new toggle, and split up the client and server toggles.

JIRA Issues: CSL-5880

Differential Revision: https://phabricator.twitter.biz/D130988

Summary: Problem

We sometimes send an RST after we receive an RST, which is explicitly disallowed
by the spec.  Although we don't seem to be getting into RST loops, it's possible
if both the client and the server are badly behaved, so we should avoid having
badly behaved clients.

Solution

Mark the transport as Dead so that it knows it doesn't need to RST.

JIRA Issues: CSL-5879

Differential Revision: https://phabricator.twitter.biz/D130963

Summary: Problem

We are leaking streams in the AdapterProxyChannelHandler when we send or receive
an RST.

Solution

Make sure to tear down a stream when we send or receive an RST.  If there's a
race and we later receive a message for a stream that has already received an
RST, we drop the message.

JIRA Issues: CSL-5872

Differential Revision: https://phabricator.twitter.biz/D130464

Summary: Problem

Right now, it's impossible to disable http/2 if it's toggled up.

Solution

Check if http/2 has been explicitly enabled or disabled, and ignore the toggle
in that case.  I also made a helper method to make it easy to enable to disable
HTTP/2.

JIRA Issues: CSL-5809

Differential Revision: https://phabricator.twitter.biz/D129068

Summary: Problem

Folks see increased rates of 431s when migrating from http/1.1 to http/2.
This seems to be because of a bug in netty which miscounts the size of headers
by double counting header names.

See netty/netty#7511.

Solution

The longterm fix is to fix netty, but the short term is to bump the max header
list size.

JIRA Issues: CSL-5725

Differential Revision: https://phabricator.twitter.biz/D121912

Summary: Problem

http/2 users who are on JDK < 9 who want ALPN need to bring their own ALPN
implementation.

Solution

Supply tcnative-boringssl with finagle-http2 by default.

JIRA Issues: CSL-5393

Differential Revision: https://phabricator.twitter.biz/D119562

Summary: Problem

Right now our ALPN downgrade drops the old connection on the ground and then
makes a new http/1.1 connection

Solution

Instead of dropping the old connection on the ground, change the pipeline to
http/1.1 and continue to use it.

Result

We no longer leak connections when the ALPN upgrade fails.

JIRA Issues: CSL-5425

Differential Revision: https://phabricator.twitter.biz/D118687

Summary: Problem

We aren't stripping the special header http2-settings from our http requests.
This header should only exist for the upgrade request, and it isn't necessary
after the upgrade has completed.

Solution

We should strip it, and we should generally santize both incoming and outgoing
requests for both the client and the server.  I also split out the sanitization
into its own channel handler, so it's a bit cleaner now.

JIRA Issues: CSL-5644

Differential Revision: https://phabricator.twitter.biz/D117676

…Nack support"

Summary: This reverts the commit for PHAB_ID=D111722

Differential Revision: https://phabricator.twitter.biz/D116539

Summary: Problem

Connections aren't being closed properly in http/2 when the client shuts down.

Solution

Propagate it all the way in AdapterProxyChannelHandler.

JIRA Issues: CSL-5425

Differential Revision: https://phabricator.twitter.biz/D111678

Differential Revision: https://phabricator.twitter.biz/D112670

Summary: Problem

There seems to be a bug with sbt 1.0.x, sbt-pgp, and coursier.
See coursier/coursier#694

Solution

Stop using coursier to unblock publishing.

JIRA Issues: CSL-5520

Differential Revision: https://phabricator.twitter.biz/D112092