Clone
 

scott mitchell <scott_mitchell@apple.com> in Netty

SnappyFrameDecoderTest ByteBuf leak (#9854)

Motivation:

SnappyFrameDecoderTest has a few tests which fail to close the EmbeddedChannel

and therefore may leak ByteBuf objects.

Modifications:

- Make sure EmbeddedChannel#finishAndReleaseAll() is called in all tests

Result:

No more leaks from SnappyFrameDecoderTest.

SnappyFrameDecoderTest ByteBuf leak (#9854)

Motivation:

SnappyFrameDecoderTest has a few tests which fail to close the EmbeddedChannel

and therefore may leak ByteBuf objects.

Modifications:

- Make sure EmbeddedChannel#finishAndReleaseAll() is called in all tests

Result:

No more leaks from SnappyFrameDecoderTest.

EPOLL - decouple schedule tasks from epoll_wait life cycle (#7834)

Motivation:

EPOLL supports decoupling the timed wakeup mechanism from the selector call. The EPOLL transport takes advantage of this in order to offer more fine grained timer resolution. However we are current calling timerfd_settime on each call to epoll_wait and this is expensive. We don't have to re-arm the timer on every call to epoll_wait and instead only have to arm the timer when a task is scheduled with an earlier expiration than any other existing scheduled task.

Modifications:

- Before scheduled tasks are added to the task queue, we determine if the new

duration is the soonest to expire, and if so update with timerfd_settime. We

also drain all the tasks at the end of the event loop to make sure we service

any expired tasks and get an accurate next time delay.

- EpollEventLoop maintains a volatile variable which represents the next deadline to expire. This variable is modified inside the event loop thread (before calling epoll_wait) and out side the event loop thread (immediately to ensure proper wakeup time).

- Execute the task queue before the schedule task priority queue. This means we

may delay the processing of scheduled tasks but it ensures we transfer all

pending tasks from the task queue to the scheduled priority queue to run the

soonest to expire scheduled task first.

- Deprecate IORatio on EpollEventLoop, and drain the executor and scheduled queue on each event loop wakeup. Coupling the amount of time we are allowed to drain the executor queue to a proportion of time we process inbound IO may lead to unbounded queue sizes and unpredictable latency.

Result:

Fixes https://github.com/netty/netty/issues/7829

- In most cases this results in less calls to timerfd_settime

- Less event loop wakeups just to check for scheduled tasks executed outside the event loop

- More predictable executor queue and scheduled task queue draining

- More accurate and responsive scheduled task execution

SslHandler to fail handshake and pending writes if non-application write fails (#9240)

Motivation:

SslHandler must generate control data as part of the TLS protocol, for example

to do handshakes. SslHandler doesn't capture the status of the future

corresponding to the writes when writing this control (aka non-application

data). If there is another handler before the SslHandler that wants to fail

these writes the SslHandler will not detect the failure and we must wait until

the handshake timeout to detect a failure.

Modifications:

- SslHandler should detect if non application writes fail, tear down the

channel, and clean up any pending state.

Result:

SslHandler detects non application write failures and cleans up immediately.

SslHandler to fail handshake and pending writes if non-application write fails (#9240)

Motivation:

SslHandler must generate control data as part of the TLS protocol, for example

to do handshakes. SslHandler doesn't capture the status of the future

corresponding to the writes when writing this control (aka non-application

data). If there is another handler before the SslHandler that wants to fail

these writes the SslHandler will not detect the failure and we must wait until

the handshake timeout to detect a failure.

Modifications:

- SslHandler should detect if non application writes fail, tear down the

channel, and clean up any pending state.

Result:

SslHandler detects non application write failures and cleans up immediately.

HTTP/2 avoid closing connection when writing GOAWAY (#9227)

Motivation:

b4e3c12b8e8e984ba65330dd6dc34a4b3d07a25a introduced code to avoid coupling

close() to graceful close. It also added some code which attempted to infer when

a graceful close was being done in writing of a GOAWAY to preserve the

"connection is closed when all streams are closed behavior" for the child

channel API. However the implementation was too overzealous and may preemptively

close the connection if there are not currently any open streams (and close if

there are any frames which create streams in flight).

Modifications:

- Decouple writing a GOAWAY from trying to infer if a graceful close is being

done and closing the connection. Even if we could enhance this logic (e.g.

wait to close until the second GOAWAY with no error) it is possible the user

doesn't want the connection to be closed yet. We can add a means for the codec

to orchestrate the graceful close in the future (e.g. write some special "close

the connection when all streams are closed") but for now we can just let the

application handle this.

Result:

Fixes https://github.com/netty/netty/issues/9207

HTTP/2 avoid closing connection when writing GOAWAY (#9227)

Motivation:

b4e3c12b8e8e984ba65330dd6dc34a4b3d07a25a introduced code to avoid coupling

close() to graceful close. It also added some code which attempted to infer when

a graceful close was being done in writing of a GOAWAY to preserve the

"connection is closed when all streams are closed behavior" for the child

channel API. However the implementation was too overzealous and may preemptively

close the connection if there are not currently any open streams (and close if

there are any frames which create streams in flight).

Modifications:

- Decouple writing a GOAWAY from trying to infer if a graceful close is being

done and closing the connection. Even if we could enhance this logic (e.g.

wait to close until the second GOAWAY with no error) it is possible the user

doesn't want the connection to be closed yet. We can add a means for the codec

to orchestrate the graceful close in the future (e.g. write some special "close

the connection when all streams are closed") but for now we can just let the

application handle this.

Result:

Fixes https://github.com/netty/netty/issues/9207

Http2ConnectionHandler to allow decoupling close(..) from GOAWAY graceful close (#9094)

Motivation:

Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close

logic. This coupling means that a user would have to override

Http2ConnectionHandler#close(..) to modify the behavior, and the

Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot

override at this layer. Ideally we can totally decouple the close(..) of the

transport and the GOAWAY graceful closure process completely, but to preserve

backwards compatibility we can add an opt-out option to decouple where the

application is responsible for sending a GOAWAY with error code equal to

NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in

order to initiate graceful close.

Modifications:

- Http2ConnectionHandler supports an additional boolean constructor argument to

opt out of close(..) going through the graceful close path.

- Http2FrameCodecBuilder and Http2MultiplexCodec expose

gracefulShutdownTimeoutMillis but do not hook them up properly. Since these

are already exposed we should hook them up and make sure the timeout is applied

properly.

- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should

initiate the graceful closure process after writing a GOAWAY frame if the error

code is NO_ERROR. This means that writing a Http2GoAwayFrame from

Http2FrameCodec will initiate graceful close.

Result:

Http2ConnectionHandler#close(..) can now be decoupled from the graceful close

process, and immediately close the underlying transport if desired.

Http2ConnectionHandler to allow decoupling close(..) from GOAWAY graceful close (#9094)

Motivation:

Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close

logic. This coupling means that a user would have to override

Http2ConnectionHandler#close(..) to modify the behavior, and the

Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot

override at this layer. Ideally we can totally decouple the close(..) of the

transport and the GOAWAY graceful closure process completely, but to preserve

backwards compatibility we can add an opt-out option to decouple where the

application is responsible for sending a GOAWAY with error code equal to

NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in

order to initiate graceful close.

Modifications:

- Http2ConnectionHandler supports an additional boolean constructor argument to

opt out of close(..) going through the graceful close path.

- Http2FrameCodecBuilder and Http2MultiplexCodec expose

gracefulShutdownTimeoutMillis but do not hook them up properly. Since these

are already exposed we should hook them up and make sure the timeout is applied

properly.

- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should

initiate the graceful closure process after writing a GOAWAY frame if the error

code is NO_ERROR. This means that writing a Http2GoAwayFrame from

Http2FrameCodec will initiate graceful close.

Result:

Http2ConnectionHandler#close(..) can now be decoupled from the graceful close

process, and immediately close the underlying transport if desired.

DefaultHeaders#valueIterator doesn't remove from the in bucket list (#9090)

Motivation:

DefaultHeaders entries maintains two linked lists. 1 for overall insertion order

and 1 for "in bucket" order. DefaultHeaders#valueIterator removal (introduced in 1d9090aab231ab737bd6459e0369b30d752296b2) only reliably

removes the entry from the overall insertion order, but may not remove from the

bucket unless the element is the first entry.

Modifications:

- DefaultHeaders$ValueIterator should track 2 elements behind the next entry so

that the single linked "in bucket" list can be patched up when removing the

previous entry.

Result:

More correct DefaultHeaders#valueIterator removal.

DefaultHeaders#valueIterator doesn't remove from the in bucket list (#9090)

Motivation:

DefaultHeaders entries maintains two linked lists. 1 for overall insertion order

and 1 for "in bucket" order. DefaultHeaders#valueIterator removal (introduced in 1d9090aab231ab737bd6459e0369b30d752296b2) only reliably

removes the entry from the overall insertion order, but may not remove from the

bucket unless the element is the first entry.

Modifications:

- DefaultHeaders$ValueIterator should track 2 elements behind the next entry so

that the single linked "in bucket" list can be patched up when removing the

previous entry.

Result:

More correct DefaultHeaders#valueIterator removal.

Http2FrameCodec to simulate GOAWAY received when stream IDs are exhausted (#9095)

Motivation:

Http2FrameCodec currently fails the write promise associated with creating a

stream with a Http2NoMoreStreamIdsException. However this means the user code

will have to listen to all write futures in order to catch this scenario which

is the same as receiving a GOAWAY frame. We can also simulate receiving a GOAWAY

frame from our remote peer and that allows users to consolidate graceful close

logic in the GOAWAY processing.

Modifications:

- Http2FrameCodec should simulate a DefaultHttp2GoAwayFrame when trying to

create a stream but the stream IDs have been exhausted.

Result:

Applications can rely upon GOAWAY for graceful close processing instead of also

processing write futures.

Http2FrameCodec to simulate GOAWAY received when stream IDs are exhausted (#9095)

Motivation:

Http2FrameCodec currently fails the write promise associated with creating a

stream with a Http2NoMoreStreamIdsException. However this means the user code

will have to listen to all write futures in order to catch this scenario which

is the same as receiving a GOAWAY frame. We can also simulate receiving a GOAWAY

frame from our remote peer and that allows users to consolidate graceful close

logic in the GOAWAY processing.

Modifications:

- Http2FrameCodec should simulate a DefaultHttp2GoAwayFrame when trying to

create a stream but the stream IDs have been exhausted.

Result:

Applications can rely upon GOAWAY for graceful close processing instead of also

processing write futures.

DefaultHttp2ConnectionEncoder async SETTINGS ACK SimpleChannelPromiseAggregator promise usage

Motivaiton:

DefaultHttp2ConnectionEncoder uses SimpleChannelPromiseAggregator to combine two

operations into a single future status. However it directly uses the

SimpleChannelPromiseAggregator object instead of using the newPromise() method

in one case. This may result in premature completion of the aggregated future.

Modifications:

- DefaultHttp2ConnectionEncoder to use

SimpleChannelPromiseAggregator#newPromise() instead of directly using the

SimpleChannelPromiseAggregator instance when writing the settings ACK frame

Result:

More correct status for the SETTING ACK frame writing when auto settings ACK is

disabled.

DefaultHttp2ConnectionEncoder async SETTINGS ACK SimpleChannelPromiseAggregator promise usage

Motivaiton:

DefaultHttp2ConnectionEncoder uses SimpleChannelPromiseAggregator to combine two

operations into a single future status. However it directly uses the

SimpleChannelPromiseAggregator object instead of using the newPromise() method

in one case. This may result in premature completion of the aggregated future.

Modifications:

- DefaultHttp2ConnectionEncoder to use

SimpleChannelPromiseAggregator#newPromise() instead of directly using the

SimpleChannelPromiseAggregator instance when writing the settings ACK frame

Result:

More correct status for the SETTING ACK frame writing when auto settings ACK is

disabled.

HTTP/2 to support asynchronous SETTINGS ACK (#9069)

Motivation:

The HTTP/2 codec will synchronously respond to a SETTINGS frame with a SETTINGS

ACK before the application sees the SETTINGS frame. The application may need to

adjust its state depending upon what is in the SETTINGS frame before applying

the remote settings and responding with an ACK (e.g. to adjust for max

concurrent streams). In order to accomplish this the HTTP/2 codec should allow

for the application to opt-in to sending the SETTINGS ACK.

Modifications:

- DefaultHttp2ConnectionDecoder should support a mode where SETTINGS frames can

be queued instead of immediately applying and ACKing.

- DefaultHttp2ConnectionEncoder should attempt to poll from the queue (if it

exists) to apply the earliest received but not yet ACKed SETTINGS frame.

- AbstractHttp2ConnectionHandlerBuilder (and sub classes) should support a new

option to enable the application to opt-in to managing SETTINGS ACK.

Result:

HTTP/2 allows for asynchronous SETTINGS ACK managed by the application.

HTTP/2 to support asynchronous SETTINGS ACK (#9069)

Motivation:

The HTTP/2 codec will synchronously respond to a SETTINGS frame with a SETTINGS

ACK before the application sees the SETTINGS frame. The application may need to

adjust its state depending upon what is in the SETTINGS frame before applying

the remote settings and responding with an ACK (e.g. to adjust for max

concurrent streams). In order to accomplish this the HTTP/2 codec should allow

for the application to opt-in to sending the SETTINGS ACK.

Modifications:

- DefaultHttp2ConnectionDecoder should support a mode where SETTINGS frames can

be queued instead of immediately applying and ACKing.

- DefaultHttp2ConnectionEncoder should attempt to poll from the queue (if it

exists) to apply the earliest received but not yet ACKed SETTINGS frame.

- AbstractHttp2ConnectionHandlerBuilder (and sub classes) should support a new

option to enable the application to opt-in to managing SETTINGS ACK.

Result:

HTTP/2 allows for asynchronous SETTINGS ACK managed by the application.

SmtpRequestEncoderTest ByteBuf leak (#9075)

Motivation:

SmtpRequestEncoderTest#testThrowsIfContentExpected has a ByteBuf leak.

Modifications:

- SmtpRequestEncoderTest#testThrowsIfContentExpected should release buffers in a finally block

Result:

No more leaks in SmtpRequestEncoderTest#testThrowsIfContentExpected.

SmtpRequestEncoderTest ByteBuf leak (#9075)

Motivation:

SmtpRequestEncoderTest#testThrowsIfContentExpected has a ByteBuf leak.

Modifications:

- SmtpRequestEncoderTest#testThrowsIfContentExpected should release buffers in a finally block

Result:

No more leaks in SmtpRequestEncoderTest#testThrowsIfContentExpected.

DefaultHeaders#valueIterator to support removal (#9063)

Motivation:

While iterating values it is often desirable to be able to remove individual

entries. The existing mechanism to do this involves removal of all entries and

conditional re-insertion which is heavy weight in order to remove a single

value.

Modifications:

- DefaultHeaders$ValueIterator supports removal

Result:

It is possible to remove entries while iterating the values in DefaultHeaders.

DefaultHeaders#valueIterator to support removal (#9063)

Motivation:

While iterating values it is often desirable to be able to remove individual

entries. The existing mechanism to do this involves removal of all entries and

conditional re-insertion which is heavy weight in order to remove a single

value.

Modifications:

- DefaultHeaders$ValueIterator supports removal

Result:

It is possible to remove entries while iterating the values in DefaultHeaders.

Correctly handle overflow in Native.kevent(...) when EINTR is detected (#9024)

Motivation:

When kevent(...) returns with EINTR we do not correctly decrement the timespec

structure contents to account for the time duration. This may lead to negative

values for tv_nsec which will result in an EINVAL and raise an IOException to

the event loop selection loop.

Modifications:

Correctly calculate new timeoutTs when EINTR is detected

Result:

Fixes #9013.

    • -1
    • +21
    /transport-native-unix-common/src/main/c/netty_unix_util.c
    • -0
    • +15
    /transport-native-unix-common/src/main/c/netty_unix_util.h
Correctly handle overflow in Native.kevent(...) when EINTR is detected (#9024)

Motivation:

When kevent(...) returns with EINTR we do not correctly decrement the timespec

structure contents to account for the time duration. This may lead to negative

values for tv_nsec which will result in an EINVAL and raise an IOException to

the event loop selection loop.

Modifications:

Correctly calculate new timeoutTs when EINTR is detected

Result:

Fixes #9013.

    • -1
    • +21
    /transport-native-unix-common/src/main/c/netty_unix_util.c
    • -0
    • +15
    /transport-native-unix-common/src/main/c/netty_unix_util.h
Correctly handle overflow in Native.kevent(...) when EINTR is detected (#9024)

Motivation:

When kevent(...) returns with EINTR we do not correctly decrement the timespec

structure contents to account for the time duration. This may lead to negative

values for tv_nsec which will result in an EINVAL and raise an IOException to

the event loop selection loop.

Modifications:

Correctly calculate new timeoutTs when EINTR is detected

Result:

Fixes #9013.

    • -1
    • +21
    /transport-native-unix-common/src/main/c/netty_unix_util.c
    • -0
    • +15
    /transport-native-unix-common/src/main/c/netty_unix_util.h
netty_unix_socket free nettyClassName after using it

Motivation:

netty_unix_socket attempts to use nettyClassName in an error message, but previously freed the memory. We should wait to free the memory until after we use it.

Modifications:

- Free nettyClassName after using it in snprintf

Result:

More useful error message.

    • -2
    • +6
    /transport-native-unix-common/src/main/c/netty_unix_socket.c
NIO read spin event loop spin when half closed

Motivation:

AbstractNioByteChannel will detect that the remote end of the socket has

been closed and propagate a user event through the pipeline. However if

the user has auto read on, or calls read again, we may propagate the

same user events again. If the underlying transport continuously

notifies us that there is read activity this will happen in a spin loop

which consumes unnecessary CPU.

Modifications:

- AbstractNioByteChannel's unsafe read() should check if the input side

of the socket has been shutdown before processing the event. This is

consistent with EPOLL and KQUEUE transports.

Result:

No more read spin loop in NIO when the channel is half closed.

EmptyHeaders get with default value returns null

Motivation:

EmptyHeaders#get with a default value argument returns null. It should never return null, and instead it should return the default value.

Modifications:

- EmptyHeaders#get with a default value should return that default value

Result:

More correct implementation of the Headers API.

Epoll flush/writabilityChange deadlock

Motivation:

b215794de31f28355e4469fcc04782f55076c80c recently introduced a change in behavior where writeSpinCount provided a limit for how many write operations were attempted per flush operation. However when the write quantum was meet the selector write flag was not cleared, and the channel unsafe flush0 method has an optimization which prematurely exits if the write flag is set. This may lead to no write progress being made under the following scenario:

- flush is called, but the socket can't accept all data, we set the write flag

- the selector wakes us up because the socket is writable, we write data and use the writeSpinCount quantum

- we then schedule a flush() on the EventLoop to execute later, however it the flush0 optimization prematurely exits because the write flag is still set

In this scenario the socket is still writable so the EventLoop may never notify us that the socket is writable, and therefore we may never attempt to flush data to the OS.

Modifications:

- When the writeSpinCount quantum is exceeded we should clear the selector write flag

Result:

Fixes https://github.com/netty/netty/issues/7729

Simplify CharSequenceValueConverter#convertToBoolean

Motivation:

CharSequenceValueConverter#convertToBoolean has a few manual conditionals which can be removed if we use AsciiString.contentEqualsIgnoreCase. Also by comparing an AsciiString to a String we will incur conversions to char that can be avoided if we compare against AsciiString.

Modifications:

- Use AsciiString.contentEqualsIgnoreCase

- Compare against a AsciiString

Result:

Simplified CharSequenceValueConverter#convertToBoolean which favors AsciiString comparison.

Reduce the default number of objects retained by the Recycler per thread

Motivation:

The Recycler currently retains 32k objects per thread by default. The Recycler is used in more than just one place and may result in large amounts of memory bloat if spikes of traffic are observed.

Modifications:

- Reduce the Recyclers default capacity from 32k to 4k.

Result:

- Lower default capacity of the Recycler and less memory retained.