Update Netty to 4.1.16.Final #28345

jaymode · 2018-01-23T19:43:49Z

This commit updates netty to 4.1.16.Final. This is the latest version that we can have work without
needing extra security permissions. This updated version of netty fixes issues seen with Java 9 and some data not being sent, which results in timeouts.

This commit updates netty to 4.1.18.Final. This is the latest version that we can have work with the security manager. This updated version of netty fixes issues seen with Java 9 and some data not being sent, which results in timeouts.

rjernst · 2018-01-23T19:51:29Z

modules/transport-netty4/src/main/plugin-metadata/plugin-security.policy

@@ -17,6 +17,11 @@
 * under the License.
 */

+grant {
+    // needed for Netty since they use reflection in ThrowableUtil :(
+    permission java.lang.RuntimePermission "accessDeclaredMembers";


Can we open an issue in netty and link here that netty should work gracefully if it does not have this permission?

Opened netty/netty#7614 and added in the comment of this section

jaymode · 2018-01-23T19:53:04Z

Netty 4.1.20.Final is available but there are a few issues that prevent us from upgrading. The main issue is in the GlobalEventExecutor. The startThread() method changes the thread's context class loader and does not use a doPrivileged block. This executor is used in a hardcoded fashion throughout the netty code so we cannot perform the wrapping for Netty (as a temporary workaround).

The ObjectCleaner class also has a similar issue in that the classloader is changed without a doPrivileged block and can be executed outside of our control.

jasontedor

I left a comment.

jasontedor · 2018-01-23T21:46:02Z

modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/Netty4Utils.java

-
+        // TODO remove this once we get a fix into Netty so we don't have to do this
+        AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
+            ThrowableUtil.haveSuppressed();


Would you leave a comment that this is triggering class initialization? This is horrific.

I pushed b073780

jasontedor

LGTM.

jasontedor

Sorry after reviewing the implications of this I left another comment.

jasontedor · 2018-01-23T22:01:22Z

modules/transport-netty4/src/main/plugin-metadata/plugin-security.policy

@@ -17,6 +17,12 @@
 * under the License.
 */

+grant {


Can we grant this only to transport-netty4?

I am pretty sure this is handled by the module framework?

That is correct. A general grant means all the jars in this plugin.

Not quite; it grants it to all JARs in the module and I am asking for this to be limited to transport-netty4 only.

Or whatever Netty JAR this is in.

That can't work since the doPriv block is in our module code.

It could be granted to our jar and the single netty jar explicitly though.

Yeah, that should have been an and, not an or.

Tim-Brooks

LGTM - as I am good with whatever decision you, Jason, and Ryan make in regards to permissions.

jaymode · 2018-01-25T16:40:47Z

@jasontedor @rjernst I rolled the version back to 4.1.16.Final to avoid needing extra security permissions and the initialization hack. Can you please take a look?

rjernst

LGTM, just one ask.

rjernst · 2018-01-25T18:35:15Z

modules/transport-netty4/build.gradle

+        'io.netty.util.internal.shaded.org.jctools.queues.BaseMpscLinkedArrayQueueProducerFields',
+        'io.netty.util.internal.shaded.org.jctools.queues.MpscArrayQueueConsumerIndexField',
+        'io.netty.util.internal.shaded.org.jctools.queues.MpscArrayQueueProducerIndexField',
+        'io.netty.util.internal.shaded.org.jctools.queues.MpscArrayQueueProducerLimitField',


Seems like these got moved out of alphabetical order?

jasontedor · 2018-01-25T19:04:38Z

Thanks @jaymode. When @rjernst comment is addressed you can merge.

This commit updates netty to 4.1.16.Final. This is the latest version that we can have work without extra permissions. This updated version of netty fixes issues seen with Java 9 and some data not being sent, which results in timeouts.

* master: (23 commits) Update Netty to 4.1.16.Final (elastic#28345) Fix peer recovery flushing loop (elastic#28350) REST high-level client: add support for exists alias (elastic#28332) REST high-level client: move to POST when calling API to retrieve which support request body (elastic#28342) Add Indices Aliases API to the high level REST client (elastic#27876) Java Api clean up: remove deprecated `isShardsAcked` (elastic#28311) [Docs] Fix explanation for `from` and `size` example (elastic#28320) Adapt bwc version after backport elastic#28358 Always return the after_key in composite aggregation response (elastic#28358) Adds test name to MockPageCacheRecycler exception (elastic#28359) Adds a note in the `terms` aggregation docs regarding pagination (elastic#28360) [Test] Fix DiscoveryNodesTests.testDeltas() (elastic#28361) Update packaging tests to work with meta plugins (elastic#28336) Remove Painless Type from MethodWriter in favor of Java Class. (elastic#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (elastic#28348) Reindex: Shore up rethrottle test Only assert single commit iff index created on 6.2 isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (elastic#27766) Fix GeoDistance query example (elastic#28355) ...

Update Netty to 4.1.18.Final

355964a

This commit updates netty to 4.1.18.Final. This is the latest version that we can have work with the security manager. This updated version of netty fixes issues seen with Java 9 and some data not being sent, which results in timeouts.

jaymode added review :Distributed Coordination/Network Http and internode communication implementations v7.0.0 v6.3.0 v6.2.0 labels Jan 23, 2018

jaymode requested a review from jasontedor January 23, 2018 19:43

rjernst reviewed Jan 23, 2018

View reviewed changes

jaymode mentioned this pull request Jan 23, 2018

ThrowableUtil should work without needing reflection permissions netty/netty#7614

Closed

add netty issue

8ef7447

jaymode requested a review from Tim-Brooks January 23, 2018 20:43

jasontedor reviewed Jan 23, 2018

View reviewed changes

add comment about class init

b073780

jasontedor approved these changes Jan 23, 2018

View reviewed changes

jasontedor requested changes Jan 23, 2018

View reviewed changes

Tim-Brooks approved these changes Jan 23, 2018

View reviewed changes

jaymode added 4 commits January 24, 2018 12:56

Merge branch 'master' into netty_update

20b346c

Merge branch 'master' into netty_update

dd5116b

Go back to Netty 4.1.16 so we don't need more permissions or the hack

c157fd6

unnecessary change

3b5a838

jaymode changed the title ~~Update Netty to 4.1.18.Final~~ Update Netty to 4.1.16.Final Jan 25, 2018

rjernst approved these changes Jan 25, 2018

View reviewed changes

alphabetical order

d81c998

jaymode merged commit e59f14d into elastic:master Jan 25, 2018

jaymode deleted the netty_update branch January 25, 2018 19:48

jpountz added the >upgrade label Jun 13, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Netty to 4.1.16.Final #28345

Update Netty to 4.1.16.Final #28345

jaymode commented Jan 23, 2018 •

edited

Loading

rjernst Jan 23, 2018

jaymode Jan 23, 2018

jaymode commented Jan 23, 2018

jasontedor left a comment

jasontedor Jan 23, 2018

jaymode Jan 23, 2018

jasontedor left a comment

jasontedor left a comment

jasontedor Jan 23, 2018

jaymode Jan 23, 2018

rjernst Jan 23, 2018

jasontedor Jan 23, 2018

jasontedor Jan 23, 2018

rjernst Jan 23, 2018

rjernst Jan 23, 2018

jasontedor Jan 23, 2018

Tim-Brooks left a comment •

edited

Loading

jaymode commented Jan 25, 2018

rjernst left a comment

rjernst Jan 25, 2018

jasontedor Jan 25, 2018

jasontedor commented Jan 25, 2018

Update Netty to 4.1.16.Final #28345

Update Netty to 4.1.16.Final #28345

Conversation

jaymode commented Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode commented Jan 23, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks left a comment • edited Loading

Choose a reason for hiding this comment

jaymode commented Jan 25, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor commented Jan 25, 2018

jaymode commented Jan 23, 2018 •

edited

Loading

Tim-Brooks left a comment •

edited

Loading