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

New implementation of PublisherInputStream that improves performance and fixes race conditions #1690

Merged
merged 3 commits into from
Apr 27, 2020

Conversation

spericas
Copy link
Member

@spericas spericas commented Apr 24, 2020

New implementation of PublisherInputStream, now renamed to DataChunkInputStream. This new implementation fixes a couple of important problems:

  1. It implements the ability to read into byte arrays, not just one byte at a time for better performance and,
  2. It changes the implementation to avoid a race condition when accessing the data chunks held in CompletableFuture's. The race condition resulted in certain tests to hang if a thread raced and updated the value of the old processed variable.

There is also a new internal document docs-internal/datachunkinputstream.md that describes the new implementation. Credit to Oleksandr Otenko.

Signed-off-by: Santiago Pericasgeertsen santiago.pericasgeertsen@oracle.com

…InputStream as it really implements a subscriber not a publisher. This new implementation fixes a couple of important problems: (1) It implements the ability to read into byte arrays, not just one byte a time for better performance and (2) It changes the implementation to avoid a race condition when accessing the data chunks held in CompletableFuture's. The race condition resulted in certain tests to hang if a thread raced and updated the value of the old 'processed' variable.

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>
@danielkec
Copy link
Contributor

How about to rename it to something completely different as it is not a publisher nor subscriber.

public class FlowInputStream extends InputStream{
...
 private class DataChunkSubscriber implements Flow.Subscriber<DataChunk>{
     public void on...
     public void on...
     public void on...
     public void on...
     ...
    }
}

To avoid users of the api to accidentally violate rule §1.10 like this:

SubscriberInputStream subscriber = new SubscriberInputStream(publisher);
publisher.subscribe(subscriber);

@spericas
Copy link
Member Author

I like the idea of the private class. I'll explore that.

…ent confusion with other public APIs. Also renamed and re-formatted internal document describing implementation.

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>
Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>
@spericas spericas changed the title WIP: New implementation of PublisherInputStream that improves performance and fixes race conditions New implementation of PublisherInputStream that improves performance and fixes race conditions Apr 27, 2020
docs-internal/subscriberinputstream.md Outdated Show resolved Hide resolved
- `Subscription.request` is invoked only after one chunk has been consumed
- the number of chunks requested is always 1
- Publisher fully conforms to the Flow.Publisher in the reactive-streams specification with respect to:
- total order of `onNext`/`onComplete`/`onError`
Copy link
Contributor

Choose a reason for hiding this comment

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

onSubscribe/onNext/onComplete/onError

docs-internal/subscriberinputstream.md Outdated Show resolved Hide resolved
Copy link

@olotenko olotenko left a comment

Choose a reason for hiding this comment

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

This change is focusing on correct synchronization (hand-over-hand) and reading in quantities larger than 1 byte. Looks good.

@spericas spericas merged commit 60abc97 into helidon-io:master Apr 27, 2020
@spericas spericas self-assigned this Apr 28, 2020
@spericas spericas added 2.x Issues for 2.x version branch bug Something isn't working webserver labels Apr 28, 2020
@spericas spericas added this to Normal priority in Backlog Apr 28, 2020
@romain-grecourt romain-grecourt moved this from Normal priority to Closed in Backlog May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Issues for 2.x version branch bug Something isn't working webserver
Projects
Backlog
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants