Skip to content

Support delegating task when using ReferenceCountedOpenSslEngine.#8859

Merged
normanmaurer merged 1 commit into4.1from
task_openssl
Mar 5, 2019
Merged

Support delegating task when using ReferenceCountedOpenSslEngine.#8859
normanmaurer merged 1 commit into4.1from
task_openssl

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

Motivation:

SSLEngine API has a notion of tasks that may be expensive and offload these to another thread. We did not support this when using our native implementation but can now for various operations during the handshake.

Modifications:

  • Support offloading tasks during the handshake when using our native SSLEngine implementation
  • Correctly handle the case when NEED_TASK is returned and nothing was consumed / produced yet

Result:

Be able to offload long running tasks from the EventLoop when using SslHandler with our native SSLEngine.

@normanmaurer
Copy link
Copy Markdown
Member Author

This depends on netty/netty-tcnative#430

@normanmaurer normanmaurer requested review from carl-mastrangelo, ejona86, slandelle and trustin and removed request for carl-mastrangelo February 11, 2019 10:42
@normanmaurer
Copy link
Copy Markdown
Member Author

/cc @njhill @johnou @doom369

Copy link
Copy Markdown
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although I don't think I'd be able to notice any bugs.

SSLContext.enableOcsp(ctx, isClient());
}

// TODO: Make the configurable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default on / off?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will make if off for now and allow to configure it with a System property... WDYT @ejona86 @johnou . Alternative I could allow to configure it via the OpenSslContext

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just that we want to avoid unstable code, or is there a reason to believe it would have a performance cost?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ejona86 its more to allow to test it a bit more before make it default. I also still need to do some performance tests but I suspect the impact should be almost zero.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then system property sounds good. When we are comfortable we change the default. When it is "proven" we remove the option.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I introduced the system property "io.netty.handler.ssl.openssl.useTasks"

@normanmaurer
Copy link
Copy Markdown
Member Author

@netty-bot test this please

1 similar comment
@normanmaurer
Copy link
Copy Markdown
Member Author

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member Author

Let me merge this once we release netty-tcnative 2.0.21.Final

@normanmaurer
Copy link
Copy Markdown
Member Author

@netty-bot test this please

@normanmaurer normanmaurer force-pushed the task_openssl branch 3 times, most recently from 71df563 to f1c2fe0 Compare March 1, 2019 13:55
Copy link
Copy Markdown
Member

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Motivation:

SSLEngine API has a notion of tasks that may be expensive and offload these to another thread. We did not support this when using our native implementation but can now for various operations during the handshake.

Modifications:

- Support offloading tasks during the handshake when using our native SSLEngine implementation
- Correctly handle the case when NEED_TASK is returned and nothing was consumed / produced yet

Result:

Be able to offload long running tasks from the EventLoop when using SslHandler with our native SSLEngine.
@tomerd
Copy link
Copy Markdown
Contributor

tomerd commented Mar 2, 2019

@netty-bot test this please

@normanmaurer normanmaurer merged commit 39fcdb3 into 4.1 Mar 5, 2019
normanmaurer added a commit that referenced this pull request Mar 5, 2019
)

Motivation:

SSLEngine API has a notion of tasks that may be expensive and offload these to another thread. We did not support this when using our native implementation but can now for various operations during the handshake.

Modifications:

- Support offloading tasks during the handshake when using our native SSLEngine implementation
- Correctly handle the case when NEED_TASK is returned and nothing was consumed / produced yet

Result:

Be able to offload long running tasks from the EventLoop when using SslHandler with our native SSLEngine.
@normanmaurer normanmaurer deleted the task_openssl branch March 7, 2019 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants