Skip to content
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

WebSocketTransport::close method does not make sense #81

Closed
bsergean opened this issue May 21, 2019 · 7 comments
Closed

WebSocketTransport::close method does not make sense #81

bsergean opened this issue May 21, 2019 · 7 comments

Comments

@bsergean
Copy link
Collaborator

We have 3 states, OPEN, CLOSED, CLOSING.

        // We start by doing an early out if we are not OPENED essentially
        if (_readyState == ReadyState::CLOSING || _readyState == ReadyState::CLOSED) return;


        // connection is opened, so close without sending close frame
        if (_readyState == ReadyState::OPEN)
        {
            {
                std::lock_guard<std::mutex> lock(_closeDataMutex);
                _closeCode = code;
                _closeReason = reason;
                _closeWireSize = closeWireSize;
                _closeRemote = remote;
            }
            {
                std::lock_guard<std::mutex> lock(_closingTimePointMutex);
                _closingTimePoint = std::chrono::steady_clock::now();
            }
            setReadyState(ReadyState::CLOSING);

            sendCloseFrame(code, reason);
            // wake up the poll, but do not close yet
            _socket->wakeUpFromPoll(Socket::kSendRequest);
        }

        // Why would we ever come back here ? If that function is called twice ?
        else
        {
@bsergean
Copy link
Collaborator Author

@Kumamon38 can you explain how this method work ? This code does not make sense to me.

@Dimon4eg
Copy link
Contributor

comment connection is opened, so close without sending close frame is not correct because below I see sendCloseFrame(code, reason);

@bsergean
Copy link
Collaborator Author

I like the new close unittest which looks sensible and which pass, there used to be bugs in that code, but I have to say that this code is now much more complex than it used to be and that it doesn't make much sense anymore.

@AlexandreK38
Copy link
Contributor

There is also the OPENING state, but I wasn’t sure about if we can be in. I think we can remove the else part and the function

@AlexandreK38
Copy link
Contributor

*and the comment

@AlexandreK38
Copy link
Contributor

Cleaned 🧹

@bsergean
Copy link
Collaborator Author

This is much better now, closing. Thanks for the fix.

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

No branches or pull requests

3 participants