Skip to content

Refactor of TCP transport.#134

Merged
NiteshKant merged 1 commit intorsocket:masterfrom
NiteshKant:local-split
Jul 7, 2016
Merged

Refactor of TCP transport.#134
NiteshKant merged 1 commit intorsocket:masterfrom
NiteshKant:local-split

Conversation

@NiteshKant
Copy link
Copy Markdown
Contributor

Problem

This is a split of PR #106 to make local transport use TCP.
There seems to be some issue in netty's local transport which creates race-conditions and causes test failures.
These refactor has some changes that are not necessarily related to local transport but makes the usage better.

Modification

Refactored common test classes to reactivesocket-test module so they can be used in both tcp and local transport without code duplication.
Added PayloadImpl as a utility implementation of Payload.

Problem

This is a split of PR rsocket#106 to make local transport use TCP.
There seems to be some issue in netty's local transport which creates race-conditions and causes test failures.
These refactor has some changes that are not necessarily related to local transport but makes the usage better.

Modification

Refactored common test classes to `reactivesocket-test` module so they can be used in both tcp and local transport without code duplication.
Added `PayloadImpl` as a utility implementation of `Payload`.
@NiteshKant
Copy link
Copy Markdown
Contributor Author

Since this is a subset of PR #106 which got a 👍 I am just merging this now.

@NiteshKant NiteshKant merged commit 3a2151f into rsocket:master Jul 7, 2016
@NiteshKant NiteshKant deleted the local-split branch July 7, 2016 23:21
@yschimke
Copy link
Copy Markdown
Member

yschimke commented Jul 8, 2016

Are you sure the issues for Local Transport were not because you were reusing the same local DuplexConnection over all tests?

I haven't noticed any issues so far but I was using the test name in the config for each test, so never reusing connections.

@NiteshKant
Copy link
Copy Markdown
Contributor Author

@yschimke ClientSetupRule reconnects to a new socket (new ID generated randomly) for every test. The code is here and here

So, it doesn't use the same connection AFAICT.

@yschimke
Copy link
Copy Markdown
Member

yschimke commented Jul 8, 2016

I was looking at the tests before changes, but might be the wrong tests :(

public class ClientServerTest {

    static ReactiveSocket client;

    static ReactiveSocket server;

    @BeforeClass
    public static void setup() throws Exception {
        LocalServerReactiveSocketConnector.Config serverConfig = new LocalServerReactiveSocketConnector.Config("test", new ConnectionSetupHandler() {
            @Override
            public RequestHandler apply(ConnectionSetupPayload setupPayload, ReactiveSocket rs) throws SetupException {
                return new RequestHandler() {
                    @Override

NiteshKant added a commit to NiteshKant/reactivesocket-java that referenced this pull request Jul 10, 2016
Problem

This is a split of PR rsocket#106 to make local transport use TCP.
There seems to be some issue in netty's local transport which creates race-conditions and causes test failures.
These refactor has some changes that are not necessarily related to local transport but makes the usage better.

Modification

Refactored common test classes to `reactivesocket-test` module so they can be used in both tcp and local transport without code duplication.
Added `PayloadImpl` as a utility implementation of `Payload`.
ilayaperumalg pushed a commit to ilayaperumalg/rsocket-java that referenced this pull request Dec 26, 2017
Adding text as suggested by Todd that permits an implementation to move management of streamId to the underlying transport.
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.

2 participants