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

EventBusBridge does not cleanup if websocket is terminated exceptionally leading to memory leak #1986

Closed
ikstewa opened this issue Jun 24, 2021 · 5 comments · Fixed by #1987
Assignees
Labels
Milestone

Comments

@ikstewa
Copy link
Contributor

ikstewa commented Jun 24, 2021

Version

4.0.3, 4.1.0

Context

When using the SockJSBridge, if a websocket is terminated without a close message the idle timeout timer is never cleaned up. As a result the SOCKET_IDLE events will be dispatched indefinitely.

Do you have a reproducer?

I'm unable to create a reproducer as it requires an unhealthy termination of the socket.
Possibly similar to reproducer here: #1676

When a websocket is terminated exceptionally it calls the close and exception handlers but does not call the end handler. The EventBusBridgeImpl registers the cleanup on the endHandler, but not on the exceptionHandler. (SockJSSocket does not have a closedHandler).
https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/handler/sockjs/impl/EventBusBridgeImpl.java#L333

Steps to reproduce

  1. Connect a websocket through the SockJS bridge.
  2. Terminate the socket (so a close is not sent).
  3. Observe the idle handler is still called every pingTimeout milliseconds.
@ikstewa ikstewa added the bug label Jun 24, 2021
@pmlopes pmlopes self-assigned this Jun 25, 2021
@pmlopes pmlopes added this to the 4.1.1 milestone Jun 25, 2021
@pmlopes
Copy link
Member

pmlopes commented Jun 25, 2021

@ikstewa @vietj it seems we miss an enum value for BridgeEventType.SOCKET_EXCEPTION as a type of event, as SOCKET_CLOSED assumes that the socket was closed properly, right?

@pmlopes
Copy link
Member

pmlopes commented Jun 25, 2021

@ikstewa can you verify if the fixes proposed in the PR's above solve the bug? I think they should but like you said it's hard to automate.

@ikstewa
Copy link
Contributor Author

ikstewa commented Jun 25, 2021

Yes. The changes from #1987 will cleanup the resources as we were expecting.

@pmlopes Can we confirm that a SOCKET_EXCEPTION always means the socket is closed? I.e. the terminal state for a socket is either SOCKET_CLOSED or SOCKET_EXCEPTION?

@pmlopes
Copy link
Member

pmlopes commented Jun 25, 2021

Ok we can disambiguate the state by keep using close but we pass "exception" : true in the event body

pmlopes added a commit that referenced this issue Jun 28, 2021
Clean up bridge state on socket exception
@ikstewa
Copy link
Contributor Author

ikstewa commented Jul 13, 2021

@pmlopes Apologies I didn't notice this earlier. The PR fixed the issue however it is logging an 'error' when this occurs. I don't believe this should be considered an error as we can't always assume a client closes the websocket connection properly.

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

Successfully merging a pull request may close this issue.

2 participants