New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to configure SslHandler to wait for close_notify response before closing the Channel and fix racy flush close_notify timeout scheduling. #6257

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
4 participants
@normanmaurer
Member

normanmaurer commented Jan 20, 2017

Motivation:

SslHandler closed the channel as soon as it was able to write out the close_notify message. This may not be what the user want as it may make sense to only close it after the actual response to the close_notify was received in order to guarantee a clean-shutdown of the connection in all cases.

Beside this closeNotifyFlushTimeoutMillis is volatile so may change between two reads. We need to cache it in a local variable to ensure it not change int between. Beside this we also need to check if the flush promise was complete the schedule timeout as this may happened but we were not able to cancel the timeout yet. Otherwise we will produce an missleading log message.

Modifications:

- Add new setter / getter to SslHandler which allows to specify the behavior (old behavior is preserved as default)
- Added unit test.
- Cache volatile closeNotifyTimeoutMillis.
- Correctly check if flush promise was complete before we try to forcibly close the Channel and log a warning.
- Add missing javadocs.

Result:

More clean shutdown of connection possible when using SSL and fix racy way of schedule close_notify flush timeouts and javadocs.
@trustin

LGTM once the comments from @Scottmitch is addressed.

Allow to configure SslHandler to wait for close_notify response befor…
…e closing the Channel and fix racy flush close_notify timeout scheduling.

Motivation:

SslHandler closed the channel as soon as it was able to write out the close_notify message. This may not be what the user want as it may make sense to only close it after the actual response to the close_notify was received in order to guarantee a clean-shutdown of the connection in all cases.

Beside this closeNotifyFlushTimeoutMillis is volatile so may change between two reads. We need to cache it in a local variable to ensure it not change int between. Beside this we also need to check if the flush promise was complete the schedule timeout as this may happened but we were not able to cancel the timeout yet. Otherwise we will produce an missleading log message.

Modifications:

- Add new setter / getter to SslHandler which allows to specify the behavior (old behavior is preserved as default)
- Added unit test.
- Cache volatile closeNotifyTimeoutMillis.
- Correctly check if flush promise was complete before we try to forcibly close the Channel and log a warning.
- Add missing javadocs.

Result:

More clean shutdown of connection possible when using SSL and fix racy way of schedule close_notify flush timeouts and javadocs.

@normanmaurer normanmaurer changed the title from Allow to configure SslHandler to wait for close_notify response befor… to Allow to configure SslHandler to wait for close_notify response before closing the Channel and fix racy flush close_notify timeout scheduling. Jan 21, 2017

@normanmaurer normanmaurer reopened this Jan 21, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 21, 2017

Member

@Scottmitch PTAL again buddy

Member

normanmaurer commented Jan 21, 2017

@Scottmitch PTAL again buddy

* forcibily.
*/
public final void setCloseNotifyFlushTimeout(long closeNotifyFlushTimeoutMillis, TimeUnit unit) {
setCloseNotifyFlushTimeoutMillis(unit.toMillis(closeNotifyFlushTimeoutMillis));

This comment has been minimized.

@johnou

johnou Jan 23, 2017

Contributor
ObjectUtil.checkNotNull(unit, "unit");
@johnou

johnou Jan 23, 2017

Contributor
ObjectUtil.checkNotNull(unit, "unit");

This comment has been minimized.

@Scottmitch

Scottmitch Jan 23, 2017

Member

we will get a NPE anyways ... more efficient to let the JVM enforce this if the condition is not expected regularly.

@Scottmitch

Scottmitch Jan 23, 2017

Member

we will get a NPE anyways ... more efficient to let the JVM enforce this if the condition is not expected regularly.

This comment has been minimized.

@johnou

johnou Jan 23, 2017

Contributor

right but at least then you know that it was unit that was null instead of checking line numbers / source and hoping that there isn't multiple objects on the one line that could have caused the NPE.

@johnou

johnou Jan 23, 2017

Contributor

right but at least then you know that it was unit that was null instead of checking line numbers / source and hoping that there isn't multiple objects on the one line that could have caused the NPE.

This comment has been minimized.

@Scottmitch

Scottmitch Jan 23, 2017

Member

I agree we may be able to provide a bit more context by manually throwing the exception but I think you ultimately have to check source code when you get this type of exception. There is only one object referenced on this line, and 1 object parameter passed into this method, so there shouldn't be any ambiguity.

@Scottmitch

Scottmitch Jan 23, 2017

Member

I agree we may be able to provide a bit more context by manually throwing the exception but I think you ultimately have to check source code when you get this type of exception. There is only one object referenced on this line, and 1 object parameter passed into this method, so there shouldn't be any ambiguity.

This comment has been minimized.

@johnou

johnou Jan 24, 2017

Contributor

just seemed a little strange to me to have the null check and then remove it.

@johnou

johnou Jan 24, 2017

Contributor

just seemed a little strange to me to have the null check and then remove it.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 23, 2017

Member

@Scottmitch @trustin I will merge this later today... thanks!

Member

normanmaurer commented Jan 23, 2017

@Scottmitch @trustin I will merge this later today... thanks!

@normanmaurer normanmaurer self-assigned this Jan 23, 2017

@normanmaurer normanmaurer added this to the 4.0.44.Final milestone Jan 23, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2017

Member

Cherry-picked into 4.1 (640ef61) and 4.0 (95f47a4)

Member

normanmaurer commented Jan 24, 2017

Cherry-picked into 4.1 (640ef61) and 4.0 (95f47a4)

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 24, 2017

Member

(branch deleted)

Member

Scottmitch commented Jan 24, 2017

(branch deleted)

@Scottmitch Scottmitch deleted the close_notify_response branch Jan 24, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2017

Member
Member

normanmaurer commented Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment