Skip to content

Fix streaming bug#405

Merged
robertroeser merged 4 commits into
rsocket:1.0.xfrom
robertroeser:fixStreamingBug
Oct 5, 2017
Merged

Fix streaming bug#405
robertroeser merged 4 commits into
rsocket:1.0.xfrom
robertroeser:fixStreamingBug

Conversation

@robertroeser

@robertroeser robertroeser commented Oct 5, 2017

Copy link
Copy Markdown
Member

There is a bug while running requestStream. There is live lock when streaming data under the correct conditions. I separated the incoming frames from out going frames. The request n from receiving prevents duplex connect from sending data out. I added a test in examples project that exercises the issue.

Supplier<ServerTransport<? extends Closeable>> serverSupplier =
() -> {
//return LocalServerTransport.create("test");
return TcpServerTransport.create(8001);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we avoid fixed ports? Can you use TcpClientSetupRule to get your client and server instead?

}

@Test(timeout = 2000)
@Ignore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why @ignore?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The test should probably be fixed, or should be redone. The TCK pasted so I ignored them for now. I think they fail because of the way they are written and would like to get this fix out.

() -> {
sendOneFrame(Frame.Cancel.from(streamId));
if (subscribedRequests != null) {
System.out.println("canceling....");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove

@yschimke yschimke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some temporary code still in place. I'll take a look at the code in place in an IDE/debugger to truly follow along. But I won't let that block you landing the fix.

.send(sendProcessor)
.doOnError(
t -> {
Collection<Subscriber<Payload>> values;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find it hard to follow any sufficiently big chain of rx operators with sizable lambdas. Can we split these up into individual methods without losing anything?

@yschimke

yschimke commented Oct 5, 2017

Copy link
Copy Markdown
Member

What frequency do you want to do releases on for bug fixes? Should I release 0.9.9, 0.9.10 for these types of fixes?

@robertroeser

robertroeser commented Oct 5, 2017

Copy link
Copy Markdown
Member Author

@yschimke I need this one kind of soon, it's blocking what I'm working on. I think 0.9.9 is fine

@yschimke

yschimke commented Oct 5, 2017

Copy link
Copy Markdown
Member

@robertroeser let me know if you want me to release. But creating the github tag should result in a staged version that we can publish from bintray, so go for it yourself if you want.

@robertroeser robertroeser merged commit f7fa6da into rsocket:1.0.x Oct 5, 2017
@robertroeser robertroeser deleted the fixStreamingBug branch October 5, 2017 17:19
@yschimke

yschimke commented Oct 5, 2017

Copy link
Copy Markdown
Member

@robertroeser nice, release seemed to work. Any problems?

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