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

[core-rest-pipeline] NodeHttpClient didn't remove EventListener correctly caused MaxListenersExceededWarning #31026

Open
MaryGao opened this issue Sep 9, 2024 · 4 comments
Open

[core-rest-pipeline] NodeHttpClient didn't remove EventListener correctly caused MaxListenersExceededWarning #31026

MaryGao opened this issue Sep 9, 2024 · 4 comments
Assignees
@mpodwysocki
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team

Comments

@MaryGao
Copy link
Member

MaryGao commented Sep 9, 2024 •
edited
Loading

  • Package Name: @azure/core-rest-pipeline
  • Package Version: latest

Describe the bug
See discussion. There are two issues

How to reproduce:

  • checkout pr Fix max listener issue #31027
  • revert core-rest-pipeline
  • rush build -t @azure-rest/ai-translation-document && rushx unit-test:node
  • you will see the MaxListenersExceededWarning

Expected behavior
The above two would cause the removal action would not be called here.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.
@github-actions github-actions bot added Azure.Core Client This issue points to a problem in the data-plane of the library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Sep 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

@deyaaeldeen @jeremymeng @joheredi @maorleger @mpodwysocki @timovv @xirzec

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Thank you for your feedback. Tagging and routing to the team member best able to assist.

@mpodwysocki mpodwysocki mentioned this issue Sep 11, 2024
Merged
3 tasks
@mpodwysocki
Copy link
Member

@MaryGao I'm trying to fix via #31077

mpodwysocki added a commit that referenced this issue Sep 11, 2024
@mpodwysocki
[core] Remove excess listeners for streams (#31077)
9eac817 
### Packages impacted by this PR

- @azure/core-rest-pipeline

### Issues associated with this PR

- #31026

### Describe the problem that is addressed by this PR

Removes excess listeners for streams to ensure that we're not adding
listeners to check for done and then not removing them.

### What are the possible designs available to address the problem? If
there are more than one possible design, why was the one in this PR
chosen?


### Are there test cases added in this PR? _(If not, why?)_


### Provide a list of related PRs _(if any)_


### Command used to generate this PR:**_(Applicable only to SDK release
request PRs)_

### Checklists
- [ ] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [ ] Added a changelog (if necessary)
@jeremymeng
Copy link
Member

@MaryGao I'm trying to fix via #31077

There's still the issue for an already completed stream? Those events will not be fired thus the promise not resolving.

@MaryGao MaryGao closed this as completed Sep 12, 2024
@MaryGao MaryGao reopened this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mpodwysocki mpodwysocki

Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jeremymeng @MaryGao @mpodwysocki