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

Examine and fix all usages of toVec() #723

Closed
AndrejMitrovic opened this issue Apr 3, 2020 · 1 comment
Closed

Examine and fix all usages of toVec() #723

AndrejMitrovic opened this issue Apr 3, 2020 · 1 comment
Labels
C.General An issue which doesn't fit in the other categories (not blockchain related) type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense
Milestone
2. Validator

Comments

@AndrejMitrovic
Copy link
Contributor

toVec returns an equivalent to std::vector which is just a set of 3 pointers. In some cases we're doing things wrongly. For example:

https://github.com/bpfkorea/agora/blob/7a99b4d82a91638f78149744530e532e4505fda6/source/agora/consensus/protocol/Nominator.d#L474

This returns pointers to GC-allocated memory back to C++, and the GC will collect the memory if it doesn't find roots pointing to it anymore.

In other cases like in #546 we had toVec() point to stack-allocated memory which went out of scope.

We should examine all usages of toVec() and fix them accordingly.
@AndrejMitrovic AndrejMitrovic added the type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense label Apr 3, 2020
@AndrejMitrovic AndrejMitrovic added this to the 2. Validator milestone Apr 3, 2020
@AndrejMitrovic AndrejMitrovic mentioned this issue Apr 6, 2020
Closed
6 tasks
@AndrejMitrovic AndrejMitrovic mentioned this issue Jun 25, 2020
Merged
@Geod24 Geod24 added the C.General An issue which doesn't fit in the other categories (not blockchain related) label Jul 5, 2020
AndrejMitrovic added a commit to AndrejMitrovic/agora that referenced this issue Aug 7, 2020
@AndrejMitrovic
Fix one more usage of toVec()
9ebd6fb 
The nominate() routine is actually recursive and is
called again by a timer, and the timer has a delegate
which captures the two previous and next values.

These need to be on the C heap rather than in D's
GC heap because it might get collected by the GC
if it doesn't contain any roots to it.

Part of bosagora#723
@AndrejMitrovic AndrejMitrovic mentioned this issue Aug 7, 2020
Merged
AndrejMitrovic added a commit to AndrejMitrovic/agora that referenced this issue Aug 7, 2020
@AndrejMitrovic
Fix one more usage of toVec()
b758191 
The nominate() routine is actually recursive and is
called again by a timer, and the timer has a delegate
which captures the two previous and next values.

These need to be on the C heap rather than in D's
GC heap because it might get collected by the GC
if it doesn't contain any roots to it.

Part of bosagora#723
linked0 pushed a commit that referenced this issue Aug 7, 2020
@AndrejMitrovic @linked0
Fix one more usage of toVec()
ef0b8e7 
The nominate() routine is actually recursive and is
called again by a timer, and the timer has a delegate
which captures the two previous and next values.

These need to be on the C heap rather than in D's
GC heap because it might get collected by the GC
if it doesn't contain any roots to it.

Part of #723
@AndrejMitrovic
Copy link
Contributor Author

All have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C.General An issue which doesn't fit in the other categories (not blockchain related) type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense
Projects
None yet
Milestone
2. Validator
Development

No branches or pull requests
2 participants
@Geod24 @AndrejMitrovic