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

feat(grpc): Support for manual duplex streaming connections for GRPC methods defined as stream <-> stream with proto service #1292

Closed

Conversation

anton-alation
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue 1286: #1286
Issue 1264: #1264

As described in both Issues ServerGrpc.createStreamServiceMethod by current design doesn't provide incoming stream for manual dispatch to a methodHandler, while it still can satisfy Client Unary -> Server Stream back type of case, full-duplex connections like (stream Message) returns (stream Message) cannot be dispatched.

What is the new behavior?

New behavior updating current design by providing framework users to decide on how to dispatch events on stream handler by passing stream handler on stream occasion directly to the method handler.

Using this approach developer can do something as following:

@Controller()
export class Controller {
    @GrpcMethod("Service")
    public async observeStream(stream: any, meta?: any): Promise<void> {
         stream.on("data", (msg:any)=> {
              console.log("Message to server", msg);
              stream.write({
                   id: "321321321321",
                   test: "some text"
              });
         });
         return null;
    }
}

If handler Promised return value is null then Framework dispatcher will treat this action as a manual control of the stream and will not try to make any further dispatch for expected Promise or Observable. Rest will stay on the side of the implementation.

If handler Promised return value is an Observable function or Promise, then behavior would be done as it was defined before this fix: Framework dispatcher expects Promise or Observable and then dispatch writes back to stream based on data presented in Observable or Promise passed back from the handler.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

… to method handler

- Will determine which type of streaming is preselected by developer and act accordingly
- Local system reports that `asyncfunction` should be used for assert
- Travis builder reports that `function` should be used for such assert

Changing back to follow Travis environment to define tests.
@coveralls
Copy link

coveralls commented Nov 15, 2018

Pull Request Test Coverage Report for Build 1322

  • 9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 93.713%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-grpc.ts 9 10 90.0%
Totals Coverage Status
Change from base Build 1312: -0.06%
Covered Lines: 2855
Relevant Lines: 2979

💛 - Coveralls

@anton-alation
Copy link
Contributor Author

@kamilmysliwiec Hi, can I ask when this PR will be merged or get into the discussion? As of now we using manual fs.writeFile replacement for node_modules/@nest/microservices/package/grpc... prior build or dev-start phases, and it's some kind of a hassle.

@kamilmysliwiec
Copy link
Member

Hi @anton-alation,
Thanks for your PR! Support for manual duplex streaming is definitely very useful, a handy feature. However, I'm afraid that the proposed API (returning null and handling the additional logic if so) is slightly imperative, not really straightforward. Perhaps, we should introduce another decorator instead? 🙂 (then, based on the decorator which has been used, enhance method with an expected logic)

@anton-alation
Copy link
Contributor Author

@kamilmysliwiec Sorry for such late answer, I was busy with holidays and wrestling Kafka :)

Let me check on how actually decorators passing values to routing templates in Nest (last time I was checking it a bit abstracted) and I will get back with some solution for that (only if you don't want to implement it by yourself).

As well on new decorator names, it can be:
@GrpcStreamPassthrough
or just
@GrpcStream
Let me know if one of those makes your heart a bit warmer ;)

Happy holidays!

@kamilmysliwiec
Copy link
Member

Sure thing, I hope you had good holidays! Either @GrpcStream or @GrpcStreamMethod would be fine I believe.

@woodcockjosh
Copy link

woodcockjosh commented Jan 26, 2019

@anton-alation any update? Really need grpc client stream. I'm totally blocked ATM. Also a decorator is nice but I really need to be able to create the client's manually too.

@xdire
Copy link

xdire commented Jan 27, 2019

@woodcockjosh we using this branch code for us already 2 months. I will be providing gists later to patch the code, and will be updating this PR to work through annotations next week.

Sorry that it took so long, we been struggling with kafka-node driver issues so I was busy making it all work together nicely :)

@anton-alation
Copy link
Contributor Author

A bit of patience, the code is half-way done, had a bit of a lag figuring on how tests are organized. ETA next week for tested PR.

@anton-alation
Copy link
Contributor Author

@kamilmysliwiec added new PR with TS-Decorator
PR: #1568

@kamilmysliwiec
Copy link
Member

I believe that this one can be closed now then? @anton-alation

@anton-alation
Copy link
Contributor Author

I believe that this one can be closed now then? @anton-alation

Sure.

@lock
Copy link

lock bot commented Sep 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants