Skip to content

SignalR Java Client LongPolling Transport #6856

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Feb 13, 2019
Merged

SignalR Java Client LongPolling Transport #6856

merged 15 commits into from
Feb 13, 2019

Conversation

mikaelm12
Copy link
Contributor

More transports!
Issue: #5379

@Eilon Eilon added the area-signalr Includes: SignalR clients and servers label Jan 18, 2019
@mikaelm12 mikaelm12 changed the title [WIP] SignalR Java Client LongPolling Transport SignalR Java Client LongPolling Transport Jan 22, 2019
@mikaelm12 mikaelm12 mentioned this pull request Jan 22, 2019
Closed
@mikaelm12 mikaelm12 requested a review from halter73 January 23, 2019 22:39
halter73
halter73 previously requested changes Jan 24, 2019
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I would poison both the LongPollingTransport and WebSocketTransport after they've stopped so they can no longer be used. Making transports restartable is more trouble than it's worth.

halter73
halter73 reviewed Jan 24, 2019
View reviewed changes
src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/LongPollingTransport.java Outdated
this.active = false;
return Completable.error(new Exception("Failed to connect"));
} else {
this.active = true;
Copy link
Member

Choose a reason for hiding this comment

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

Does this just throw away the content of the first poll response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first response should be empty

Copy link
Member

@halter73 halter73 Jan 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It would be good to validate that. If the response is non-empty, the content should probably go in an exception that gets raised to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't something we do in any of the clients. I don't think it's necessary. But we can add it

BrennanConroy
BrennanConroy reviewed Jan 28, 2019
View reviewed changes
src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java Outdated
@@ -368,7 +381,9 @@ public void run() {
}

if (response.getRedirectUrl() == null) {
if (!response.getAvailableTransports().contains("WebSockets")) {
if ((this.transportEnum == TransportEnum.ALL && !(response.getAvailableTransports().contains("WebSockets") || response.getAvailableTransports().contains("LongPolling"))) ||
Copy link
Member

Choose a reason for hiding this comment

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

We should cleanup this if statement

@mikaelm12
Copy link
Contributor Author

I think this is almost ready to go in.

@mikaelm12
Copy link
Contributor Author

Ping

BrennanConroy
BrennanConroy requested changes Feb 5, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

You can't add a whole feature without tests

BrennanConroy
BrennanConroy reviewed Feb 6, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Do a formatting pass.

Also, you should manually test that the LongPolling stuff doesn't change the API level

BrennanConroy
BrennanConroy reviewed Feb 7, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Real close now 👍

BrennanConroy
BrennanConroy reviewed Feb 8, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Look through the logs and add full stops where they're missing and check the log levels.

Also, should add a test for the auth tokens being passed through correctly.

And you have a couple flaky tests because you need to wait for the polls to finish before doing some checks.

BrennanConroy
BrennanConroy approved these changes Feb 11, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Couple comments and tests you should add, then good to go

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/LongPollingTransport.java Outdated
private Completable poll(String url) {
if (this.active) {
pollUrl = url + "&_=" + System.currentTimeMillis();
logger.debug("Polling {}", pollUrl);
Copy link
Member

Choose a reason for hiding this comment

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

nit: full stop

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/LongPollingTransport.java Outdated
} else {
logger.debug("Long Polling transport polling complete.");
receiveLoop.onComplete();
if (!stopCalled) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't being set anywhere.
A bool isn't very thread safe, might want to consider an atomic or something else.
Add a test that the 204 calls onclose and afterwards stop doesn't.
And another for calling stop and not getting 2 onclose callbacks.

...gnalR/clients/java/signalr/src/test/java/com/microsoft/signalr/LongPollingTransportTest.java Outdated

assertFalse(onClosedRan.get());
transport.start("http://example.com").timeout(100, TimeUnit.SECONDS).blockingAwait();
block.blockingAwait();
Copy link
Member

Choose a reason for hiding this comment

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

These blocks should have timeouts

...gnalR/clients/java/signalr/src/test/java/com/microsoft/signalr/LongPollingTransportTest.java Outdated
});

assertFalse(onClosedRan.get());
transport.start("http://example.com").timeout(100, TimeUnit.SECONDS).blockingAwait();
Copy link
Member

Choose a reason for hiding this comment

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

This became 100 seconds again

...gnalR/clients/java/signalr/src/test/java/com/microsoft/signalr/LongPollingTransportTest.java Outdated
onClosedRan.set(true);
assertEquals("Unexpected response code 999.", error);
blocker.onComplete();

Copy link
Member

Choose a reason for hiding this comment

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

nit: newline

...gnalR/clients/java/signalr/src/test/java/com/microsoft/signalr/LongPollingTransportTest.java Outdated
transport.setOnClose((error) -> {});

transport.start("http://example.com").timeout(1, TimeUnit.SECONDS).blockingAwait();
blocker.blockingAwait(1, TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

I think if you use the timeout overload of blockingAwait you need to assert true on it

@BrennanConroy
Copy link
Member

you should manually test that the LongPolling stuff doesn't change the API level

Have you done this?

BrennanConroy
BrennanConroy reviewed Feb 13, 2019
View reviewed changes
src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/LongPollingTransport.java

@Override
public Completable stop() {
if (!stopCalled.get()) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this isn't thread safe

@mikaelm12
Copy link
Contributor Author

androidapi19test
The project is called API23Test but the device is running API 19 👍

@mikaelm12 mikaelm12 dismissed halter73’s stale review February 13, 2019 18:20

Approved by Brecon

@BrennanConroy
Copy link
Member

Can you try 16? That's what we claim to support

@mikaelm12
Copy link
Contributor Author

Oh, thought it was 19. Just verified 16 as well.

@mikaelm12 mikaelm12 merged commit 3d3ad96 into master Feb 13, 2019
@natemcmaster natemcmaster deleted the mikaelm12/LPT branch May 3, 2019 06:26
@BrennanConroy BrennanConroy mentioned this pull request Sep 19, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 left review comments

@JonathanGiles JonathanGiles JonathanGiles left review comments

@BrennanConroy BrennanConroy BrennanConroy approved these changes

Assignees
No one assigned
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mikaelm12 @BrennanConroy @halter73 @JonathanGiles @Eilon