Ensure we always update the promise before calling fire* methods and …

[normanmaurer]

…also udate channel state before.

Motivation:

We not always ensure correct ordering which made it quite hard to reason about things. We should always first notify the promise before we call the fire* method that belongs to the event. Beside this we sometimes fired events or notified promised before correctly update the active state / addresses of a Channel which could result in unexpected results when query the Channel during execution of callbacks and handlers.

Modifications:

• Ensure we always notify promise first
• Always correctly update channel state before notify promise
• Add test to verify notification order.

Result:

Correct ordering of events which makes things easier to reason about and which follows what netty is doing.

[Lukasa]

Cool, so this is basically pretty good! I'd like to see this test pulled out to its own file so you can declare the lifecycle handlers outside the test function, but otherwise this is basically good (plus notes).

[Lukasa]

Wait, why are we returning here? If we fail to close the socket we don't cancel the pending writes?

[normanmaurer]

maybe we should still do ... Good question. I guess there is not much we can do in this case.

[Lukasa]

We can reduce this duplicate code by writing a pair of helper functions:

func assertFulfilled(promise: EventLoopPromise<Void>?, promiseName: String, trigger: String, setter: String, file: String = #file, line: Uint = #line) {
        if let promise = promise {
            XCTAssertTrue(promise.futureResult.fulfilled, "\(promiseName) not fulfilled before \(trigger) was called", file: file, line: line)
        } else {
            XCTFail("\(setter) not called before \(trigger)", file: file, line: line
        }
    }

This would let us truncate this file substantially and better express the criteria of these assertions.

[Lukasa]

Do you actually accept this promise being nil?

[normanmaurer]

nope it should not be null ever so I think we can just make it a force unwrap.

[Lukasa]

Or this one?

[Lukasa]

Or this one?

[normanmaurer]

@Lukasa I think I addressed all of your comments (hopefully).

[Lukasa]

This code cannot possibly be right. What's the intended behaviour here?

[normanmaurer]

ok let me explain what I wanted to do ... Basically because we fail the promise here I wanted to ensure we pass nil to becomeInactive0 so we not try to notify the promise again.

[Lukasa]

Ah, ok. Please put a comment to that effect here.

[normanmaurer]

done