StreamTransport::close() seems to be misaligned.

[thabach]

StreamTransport::close() is currently invoked after every completed GET, while it is having an implementation that asks for a one-time, terminal invocation only.

[buildhive]

Atmosphere Framework » wasync #535 UNSTABLE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Atmosphere Framework » wasync #536 UNSTABLE Looks like there's a problem with this pull request (what's this?)

[thabach]

Heya Jeanfrancois

with my change the eventing becomes compliant with what the Event enum has in its JavaDoc : /* * This event is fired when the connection gets closed. This event is only fired once. / CLOSE,

but it causes test failures now. This makes me wonder what the correct event sequence should be, a CLOSE before every REOPENED (reconnect) as described in Socket ? If that is the case and the original implementation is deemed correct, then at least close() should not shutdown() the timer and the JavaDoc in Event has to be adjusted.

This leaves me a bit confused now :), please comment.

Cheers, Christian.

[jfarcand]

@thabach Can you elaborate on the issue? With streaming, the connection will be automatically closed as soon as the server resume it. Thanks!

[thabach]

@jfarcand alright, so the current implementation is correct from an event flow perspective (and my pull request breaks the streaming event model, as detected by the Unit Test failures), still there are two problems I would like to elaborate on:

• the Event enum claims in its javadoc that a CLOSE event is supposed to come only once, while it comes once per resume. I think that javadoc should then be corrected.
• (and more importantly) the current implementation of StreamTransport::close() (callled on every resume) does a timer.shutdown(); which leads to NPEs on :

 if (options.reconnect()) {
            // We can't let the STATUS to close as fire() method won't work.
            status = Socket.STATUS.REOPENED;
            if (options.reconnectInSeconds() > 0) {
                timer.schedule(new Runnable() {
                    public void run() {
                        reconnect();
                    }
                }, options.reconnectInSeconds(), TimeUnit.SECONDS);
            }

if a reconnect timeout is configured, which is about to become an important feature in GET request modulation for us :sunglasses:

The problem really is that the intermediate close() semantics on resume do not match with terminal close() semantics (i.e., the closing of resources) while the implementation needs to serve both purposes.

It can't think of an easy solution, else I'd pull requested it by now rather than becoming lentghy again :stuck_out_tongue_winking_eye:

[buildhive]

Atmosphere Framework » wasync #537 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Atmosphere Framework » wasync #538 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Atmosphere Framework » wasync #539 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Atmosphere Framework » wasync #540 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Atmosphere Framework » wasync #541 FAILURE Looks like there's a problem with this pull request (what's this?)

[thabach]

@jfarcand nevermind the additional commits, I wanted to create another pull request "allow for subsecond GET reconnect timeouts", but they got attached to this one.

U can delete all of this pull request's commits, I just don't want to loose the conversation and (of course) am still curious about your thoughts on my problem with close().

[thabach]

@jfarcand I close this pull request because the commits are either breaking stuff, or don't apply here. But l suggest we continue our discussion on StreamTransport::close() anyways in this thread, ya ?

[jfarcand]

@thabach Hey! Sorry for the delay I'm quite swaped with Async-Io those days :-) Let's have the discussion on the mailing list. Thanks!!!!