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

Fixed memory leaks in subscriptions #3608

Merged

Conversation

akolpachev
Copy link
Contributor

Outgoing channel is closed now if related subscription or websocket is closed. MessageReceiver is stopped if error happens in linked message processor. OperationContext is marked as completed in subscribe operation.

Closes #2740
Closes #3595

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@michaelstaib
Copy link
Member

Great .... thanks for putting this together ... we will do a review tonight.

@akolpachev
Copy link
Contributor Author

Fixed tests

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@michaelstaib
Copy link
Member

michaelstaib commented Apr 27, 2021

TODO @michaelstaib

  • Subscription Diagnostics
  • Error Cases (safely abandon contexts)
  • Completion Handling
  • Formatting

@michaelstaib
Copy link
Member

@akolpachev I have a small todo list for this which I will work through tomorrow. This I great work. We will merge it once I have ticked of my items on this probably tomorrow afternoon. It will be included into the next preview.

@akolpachev
Copy link
Contributor Author

Nice. Thank you.

@tobias-tengler tobias-tengler changed the title Fixed issues #2740 and #3595 (memory leaks in subscriptions) Fixed memory leaks in subscriptions Apr 28, 2021
@michaelstaib
Copy link
Member

michaelstaib commented Apr 28, 2021

I am done with the PR. If you two (@akolpachev, @PascalSenn) would have another look through it I would be thankful. I put a lot more comments into several parts to help future contributors. Also we have no better diagnostic events and I fixed some of the naming on the transport side.

michaelstaib
michaelstaib previously approved these changes Apr 28, 2021
@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

michaelstaib
michaelstaib previously approved these changes Apr 28, 2021
@michaelstaib
Copy link
Member

Tests look good ... The sonar build error is due to a flaky test. So no worries there.

@michaelstaib
Copy link
Member

@PascalSenn I would be read to merge if you give me thumbs up.

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@michaelstaib
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@michaelstaib michaelstaib merged commit 6a79394 into ChilliCream:main Apr 28, 2021
@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants