Fixes ByteBuffer+Stream integration, TLS neg, STEE Shutdown

[nayato]

Motivation:
Fix up top priority issues to ensure proper working state with recent changes.

Modifications:

• TlsHandler negotiates TLS 1.0+ on server side (TlsHandler negotiates wrong protocols on server side #89)
• STEE properly supports graceful shutdown (SingleThreadEventExecutor shutdown takes long time #7)
• UnpooledHeapByteBuffer.GetBytes honors received index and length (TlsHandler malfunctions in conjunction with LengthFieldBasedFrameDecoder #88)
• Echo E2E test uses length-prefix based framing (Echo E2E test fails due to lack of framing and recent change in SslStream behavior #90)

Result:
Setting up DotNetty does not require workarounds for shutdown and hacks to enable negotiation of higher versions of TLS. Tests are passing even with new SslStream behavior.

[Aaronontheweb]

change to if (!inEventLoop || IsShuttingDown)

[nayato]

there's difference between IsShuttingDown is >= and WakeUp uses ==

[Aaronontheweb]

ah, right. guess there's no point in waking up after we've terminated.

[Aaronontheweb]

Seeing debug assertions get thrown for this spec in ReSharper as a result of this change:

public class SingleThreadEventExecutorSpecs
    {
        public static AtomicCounter ThreadNameCounter { get; } = new AtomicCounter(0);

        [Fact]
        public void STE_should_complete_task_when_operation_is_completed()
        {
            using (
                var executor = new SingleThreadEventExecutor("Foo" + ThreadNameCounter.GetAndIncrement(),
                    TimeSpan.FromMilliseconds(100)))
            {
                Func<bool> myFunc = () => true;
                var task = executor.SubmitAsync(myFunc);
                Assert.True(task.Wait(200), "Should have completed task in under 200 milliseconds");
                Assert.True(task.Result);
                executor.GracefulShutdownAsync();
            }
        }
}

[Aaronontheweb]

InEventLoop == false during PollTask. Don't have more time to investigate now.

[nayato]

That doesn't make sense... PollTask is run only in a context of Loop: directly or through ConfirmShutdown or through CleanupAndTerminate -> ConfirmShutdown

[Aaronontheweb]

I'm in agreement with you - I'll try to replicate this issue with the normal XUnit runner; so far I've only seen it happen in ReSharper.

[Aaronontheweb]

heh, just realized what it might be - think there could be an issue with the Dispose call on the using statement that causes a race condition here?

[Aaronontheweb]

Since I'm not blocking on the GracefulShutdownAsync ?

[nayato]

makes sense

[nayato]

Not sure if it even makes sense to have it implement IDisposable

[Aaronontheweb]

at the very least, have the IDisposable call gracefulshutdown itself and wait on it.

[nayato]

no, that's just misleading..

[nayato]

I think I introduced impl of IDisposable as a port of ExecutorService.shutdown which is really deprecated. I think it is misleading to have Dispose. IAsyncDisposable would have made sense but we're not there yet.

[nayato]

Funny part is, it didn't even implement IDisposable. Just the Dispose method was there.

[nayato]

I'm removing Dispose, will post a new version in a sec.

[Aaronontheweb]

no, that's just misleading..

I don't agree with that - I'd expect the result of the call to be that the object is finalized and ready for garbage collection when the method exits.

[Aaronontheweb]

I'm removing Dispose, will post a new version in a sec.

👍

[nayato]

Dispose is meant for lightweight teardown though. Blocking call that might not return in 5 seconds (default shutdown timeout) might result in confusion. Better have a clear non-standard way of doing shutdown for now. Once IAsyncDisposable comes around, we'll support that as it sets right expectations and allows for non-blocking signaling of shutdown completion.

[Aaronontheweb]

Dispose is meant for lightweight teardown though. Blocking call that might not return in 5 seconds (default shutdown timeout) might result in confusion.

Yeah, that's true - if the teardown takes sufficiently long that would be undesirable too. I agree with what you've done here.

[mtuchkov]

LGTM