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

(#1363) Fix memory leak in ReadEventsInternalAsync for subscriptions #1365

Merged
merged 1 commit into from Jul 28, 2017

Conversation

2 participants
@arnodenuijl
Contributor

arnodenuijl commented Jul 18, 2017

Fixes #1363

ReadEventsInternalAsync and ReadEventsCallbackAsync called each other recursively. This resulted in stackoverflowexception when subscribing to a very large stream and a lot of memory pressure.

Main fix is to create a do..while in ReadEventsInternalAsync to facilitate continuing with the next slice if not done. This in stead of the recursive call.
To make this work ReadEventsCallbackAsync must return a bool indicating if we are done (shouldstop is true OR ProcessEventsAsync indicates that we are done)

As a side effect a couple of parameters could be dropped because they were only passed between the functions but not really used.

@pgermishuys

This comment has been minimized.

Show comment
Hide comment
@pgermishuys

pgermishuys Jul 27, 2017

Member

Hi @arnodenuijl,
The formatting changes makes it difficult to spot the changes :)

Member

pgermishuys commented Jul 27, 2017

Hi @arnodenuijl,
The formatting changes makes it difficult to spot the changes :)

@pgermishuys

This comment has been minimized.

Show comment
Hide comment
@pgermishuys

pgermishuys Jul 27, 2017

Member

@arnodenuijl would it be possible to not have the formatting changes included? Sorry if it's a little picky, it's just easier to see what's changed in the future when someone else comes along?

Member

pgermishuys commented Jul 27, 2017

@arnodenuijl would it be possible to not have the formatting changes included? Sorry if it's a little picky, it's just easier to see what's changed in the future when someone else comes along?

#1363 Fix
ReadEventsInternalAsync and ReadEventsCallbackAsync called each other recursively. This resulted in stackoverflowexception when subscribing to a very large stream and a lot of memory pressure.

Main fix is to create a do..while in ReadEventsInternalAsync to facilitate continuing with the next slice if not done. This in stead of the recursive call.
To make this work ReadEventsCallbackAsync must return a bool indicating if we are done (shouldstop is true OR ProcessEventsAsync indicates that we are done)

As a side effect a couple of parameters could be dropped because they were only passed between the functions but not really used.
@arnodenuijl

This comment has been minimized.

Show comment
Hide comment
@arnodenuijl

arnodenuijl Jul 28, 2017

Contributor

No problem. I looked at the changes and fixed some tabs vs spaces. But I'm afraid this is as good as I can get it. There are some indentation differences that are caused by the do..while loop and the changes in the 'if' structure. So most of the formatting changes that are left are intentional.

I think the best way to review the change is to look at the version before and the version after and reason about both behaviours. Before it was a mutual recursive function, now it is a do..while loop. It does the same, but in a different way. But that change in behaviour doesn't show well in a text diff.

If you have any suggestions how to further clarify the changes i'm happy to give it a shot.

Contributor

arnodenuijl commented Jul 28, 2017

No problem. I looked at the changes and fixed some tabs vs spaces. But I'm afraid this is as good as I can get it. There are some indentation differences that are caused by the do..while loop and the changes in the 'if' structure. So most of the formatting changes that are left are intentional.

I think the best way to review the change is to look at the version before and the version after and reason about both behaviours. Before it was a mutual recursive function, now it is a do..while loop. It does the same, but in a different way. But that change in behaviour doesn't show well in a text diff.

If you have any suggestions how to further clarify the changes i'm happy to give it a shot.

@pgermishuys

This comment has been minimized.

Show comment
Hide comment
@pgermishuys

pgermishuys Jul 28, 2017

Member

@arnodenuijl, no worries! thank you so much for the effort!

Member

pgermishuys commented Jul 28, 2017

@arnodenuijl, no worries! thank you so much for the effort!

@pgermishuys pgermishuys merged commit 9f5467d into EventStore:release-v4.0.2 Jul 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
wercker/build-mono4 Wercker pipeline passed
Details

hayley-jean added a commit that referenced this pull request Jul 31, 2017

@hayley-jean hayley-jean changed the title from #1363 Fix to (#1363) Fix memory leak in ReadEventsInternalAsync for subscriptions Aug 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment