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.

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

fix: named pipe issues #3627

Merged
merged 9 commits into from
May 19, 2021
Merged

fix: named pipe issues #3627

merged 9 commits into from
May 19, 2021

Conversation

joshgummersall
Copy link
Contributor

@joshgummersall joshgummersall commented Apr 27, 2021 •
edited
Loading

  • Remove unnecessary autoReconnect logic
  • Simplify connection management
  • Properly support client reconnection to incoming/outgoing servers

Fixes #3472

@joshgummersall joshgummersall requested a review from a team as a code owner April 27, 2021 15:47
@joshgummersall joshgummersall marked this pull request as draft April 27, 2021 15:49
@joshgummersall
Copy link
Contributor Author

Note: this is marked as a draft while we work to ensure this resolves the issue.

@coveralls
Copy link

coveralls commented Apr 27, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 857639948

  • 83 of 89 (93.26%) changed or added relevant lines in 7 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 85.474%
Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botframework-streaming/src/utilities/createNodeServer.ts 9 10 90.0%
libraries/botframework-streaming/src/namedPipe/namedPipeServer.ts 33 38 86.84%
Files with Coverage Reduction New Missed Lines %
libraries/botbuilder/src/botFrameworkAdapter.ts 1 89.58%
libraries/botframework-streaming/src/namedPipe/namedPipeServer.ts 2 82.19%
Totals Coverage Status
Change from base Build 857493083: 0.02%
Covered Lines: 19239
Relevant Lines: 21416
💛 - Coveralls

@stevengum stevengum self-requested a review April 27, 2021 19:22
@stevengum stevengum added Area: Streaming Issues related to streaming support Automation: No parity PR does not need to be applied to other languages. labels Apr 27, 2021
DDEfromOR
DDEfromOR approved these changes Apr 28, 2021
View reviewed changes
Copy link
Member

@DDEfromOR DDEfromOR left a comment

Choose a reason for hiding this comment

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

Solid improvements, thank you Josh!

libraries/botframework-streaming/src/namedPipe/namedPipeServer.ts Show resolved Hide resolved
libraries/botframework-streaming/src/payloadTransport/payloadSender.ts Show resolved Hide resolved
@joshgummersall joshgummersall marked this pull request as ready for review April 29, 2021 23:33
@joshgummersall
Copy link
Contributor Author

This has been running stable in a sample bot for >24 hours so I think it's safe to say it works.

@joshgummersall
Copy link
Contributor Author

joshgummersall commented May 4, 2021 •
edited
Loading

A bot running with this code actually did reproduce the customer issue. Let's hold off on this (converting back to a draft for now).

EDIT: We have some confidence that this code, along with the web.config change here mitigates the issues we've seen with JS bots and Diretline ASE.

@joshgummersall joshgummersall marked this pull request as draft May 4, 2021 22:58
@joshgummersall joshgummersall marked this pull request as ready for review May 11, 2021 17:31
@stevengum stevengum mentioned this pull request May 13, 2021
Closed
stevengum
stevengum approved these changes May 14, 2021
View reviewed changes
Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

Approving again for the latest commits (101c4ec, bb7ea81).

There was one small bit of feedback around naming, but the changes in this PR make the code in around NamedPipes much, much clearer. Great work!

@joshgummersall joshgummersall force-pushed the jpg/named-pipe-cleanup branch 2 times, most recently from 466e35a to 257e868 Compare May 14, 2021 22:16
fix: named pipe issues
84d6667 
- Remove unnecessary autoReconnect logic
- Simplify connection management
- Properly support client reconnection to incoming/outgoing servers
@joshgummersall joshgummersall merged commit 09b3b07 into main May 19, 2021
@joshgummersall joshgummersall deleted the jpg/named-pipe-cleanup branch May 19, 2021 17:44
joshgummersall added a commit that referenced this pull request May 26, 2021
@joshgummersall
fix: named pipe issues (#3627)
5ff9d24 
* fix: named pipe issues

- Remove unnecessary autoReconnect logic
- Simplify connection management
- Properly support client reconnection to incoming/outgoing servers

* fix: PR comments, failing tests

* fix: "lock" sockets to mitigate split brain

* test: no split brain

* fix: reject listening only if server is listening

* fix: naming for clarity

* fix: clarify onListen doc string

* feat: retry helper, use for named pipes

* fix: handle retry edge cases
joshgummersall added a commit that referenced this pull request May 27, 2021
@joshgummersall
cherrypick: changes for 4.13.5 (#3711)
0ee95d2 
* fix: named pipe issues (#3627)

* fix: named pipe issues

- Remove unnecessary autoReconnect logic
- Simplify connection management
- Properly support client reconnection to incoming/outgoing servers

* fix: PR comments, failing tests

* fix: "lock" sockets to mitigate split brain

* test: no split brain

* fix: reject listening only if server is listening

* fix: naming for clarity

* fix: clarify onListen doc string

* feat: retry helper, use for named pipes

* fix: handle retry edge cases

* bump: adal-node to 0.2.2 (#3705)

Fixes #3704

* fix: ignore node_modules declarative assets (#3709)

See microsoft/BotFramework-Composer#7916 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdrichardson mdrichardson mdrichardson left review comments

@stevengum stevengum stevengum approved these changes

@DDEfromOR DDEfromOR DDEfromOR approved these changes

Assignees
No one assigned
Labels
Area: Streaming Issues related to streaming support Automation: No parity PR does not need to be applied to other languages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nodejs process periodic restart causing "Already connected" error from app service and 502 error from frontend
5 participants
@joshgummersall @coveralls @DDEfromOR @stevengum @mdrichardson