[SERVER-16889] Query subsystem public API should use std::unique_ptr for ownership transfer Created: 16/Jan/15 Updated: 05/Feb/16 Resolved: 06/Jul/15 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Querying |
| Affects Version/s: | None |
| Fix Version/s: | 3.1.6 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | J Rassi | Assignee: | Qingyang Chen |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | Quint Iteration 6 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
The main entry methods in the query subsystem transfer ownership of heap-allocated objects through raw pointers, which is error-prone. These methods should use std::unique_ptr instead. Partial list follows:
Linking a few tickets describing ownership-related memory leaks induced by incorrect use of the above (and related) methods. |
| Comments |
| Comment by Githook User [ 06/Jul/15 ] |
|
Author: {u'username': u'dannenberg', u'name': u'matt dannenberg', u'email': u'matt.dannenberg@10gen.com'}Message: |
| Comment by Githook User [ 06/Jul/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by Githook User [ 06/Jul/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by Githook User [ 29/Jun/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by Githook User [ 26/Jun/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by J Rassi [ 26/Jun/15 ] |
|
No, let's not consider those work items part of the scope of this ticket. Though there's plenty more work that needs to be done for the query subsystem in this area, the work already outlined is going to yield us a large benefit, and there's higher-impact work to schedule instead. |
| Comment by Qingyang Chen [ 26/Jun/15 ] |
|
Should we also do:
|
| Comment by Githook User [ 26/Jun/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by Githook User [ 23/Jun/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by Githook User [ 22/Jun/15 ] |
|
Reverting due to Code Freeze, will re-push after Code Freeze is lifted. Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: Revert " This reverts commit 3f6f66daac840fe2b2dc26eeeacbf015479567df. |
| Comment by Githook User [ 22/Jun/15 ] |
|
Author: {u'username': u'coollog', u'name': u'Qingyang Chen', u'email': u'qingyang.chen@10gen.com'}Message: |
| Comment by J Rassi [ 17/Jun/15 ] |
|
MatchExpressionParser::parse() is not, actually. That method returns a StatusWith<MatchExpression*>, whereas it should instead return StatusWith<std::unique_ptr<MatchExpression>>. It seems fine to me to change the "StatusWithMatchExpression" typedef to refer to the latter instead of the former. |
| Comment by Qingyang Chen [ 17/Jun/15 ] |
|
LiteParsedQuery::make() and MatchExpressionParser::parse() seems to be taken care of already. |