Skip to content

Conversation

@OlegDokuka
Copy link
Member

This PR provides some enhancement in WebSocket client/server API which allows manipulating connection handling + receiving a human-readable reason for closed WebSocket.

For more info see forum discussion -> https://community.netifi.com/t/accessing-http-connection-headers-and-path/180

Also related (rsocket/rsocket-js#42)

Signed-off-by: Oleh Dokuka [email protected]

@robertroeser robertroeser marked this pull request as ready for review September 5, 2019 18:41
Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: Oleh Dokuka <[email protected]>
…ty/server/WebsocketRouteTransport.java

Co-Authored-By: Yuri Schimke <[email protected]>
return isError;
}

if (headersPredicate != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be simplified by having a default headers predicate that always returns true?


public static class Builder {

private Predicate<HttpHeaders> headersPredicate;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why split headersPredicate and webSocketCloseStatusSupplier? Seems like this could be combined into a single function call instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that returning null in case everything is fine is a better approach? I was thinking about API which allows a user to filter incoming connection out and in case it is rejected to provide a specific close status. One of the options is to use SynchronousSink (as it is in Reactor) but it seems way complex as of normal developer standpoint (e.g. calling error or success)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case splitting it means possibly evaluating the rules twice, so probably. Or use optional?

@OlegDokuka
Copy link
Member Author

close in favor of #835

@OlegDokuka OlegDokuka closed this May 11, 2020
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

Successfully merging this pull request may close these issues.

4 participants