{"action":"created","issue":{"url":"https://api.github.com/repos/jcmoyer/rust-lua53/issues/65","repository_url":"https://api.github.com/repos/jcmoyer/rust-lua53","labels_url":"https://api.github.com/repos/jcmoyer/rust-lua53/issues/65/labels{/name}","comments_url":"https://api.github.com/repos/jcmoyer/rust-lua53/issues/65/comments","events_url":"https://api.github.com/repos/jcmoyer/rust-lua53/issues/65/events","html_url":"https://github.com/jcmoyer/rust-lua53/pull/65","id":174493500,"number":65,"title":"Fix origin control of a reference to a state","user":{"login":"DenisKolodin","id":418920,"avatar_url":"https://avatars.githubusercontent.com/u/418920?v=3","gravatar_id":"","url":"https://api.github.com/users/DenisKolodin","html_url":"https://github.com/DenisKolodin","followers_url":"https://api.github.com/users/DenisKolodin/followers","following_url":"https://api.github.com/users/DenisKolodin/following{/other_user}","gists_url":"https://api.github.com/users/DenisKolodin/gists{/gist_id}","starred_url":"https://api.github.com/users/DenisKolodin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DenisKolodin/subscriptions","organizations_url":"https://api.github.com/users/DenisKolodin/orgs","repos_url":"https://api.github.com/users/DenisKolodin/repos","events_url":"https://api.github.com/users/DenisKolodin/events{/privacy}","received_events_url":"https://api.github.com/users/DenisKolodin/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2016-09-01T10:51:47Z","updated_at":"2016-09-01T20:08:55Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/jcmoyer/rust-lua53/pulls/65","html_url":"https://github.com/jcmoyer/rust-lua53/pull/65","diff_url":"https://github.com/jcmoyer/rust-lua53/pull/65.diff","patch_url":"https://github.com/jcmoyer/rust-lua53/pull/65.patch"},"body":"This is **important** PR to fix potential **memory leaks** with `extra` data.\r\n\r\nI use `extra` objects a lot and found case when memory leaks:\r\n```rust\r\nlet extra = Arc::new(Mutex::new(0));\r\nlet state = State::new();\r\nstate.set_extra(Some(Box::new(extra.clone())));\r\nlet thread = state.new_thread();\r\nthread.set_extra(Some(Box::new(extra.clone())));\r\ndrop(thread); // extra pointer is lost here\r\ndrop(state);\r\n```\r\n\r\nThe reason why pointer is lost:\r\n```rust\r\npub fn new_thread(&mut self) -> State {\r\n  unsafe {\r\n    // `owned` flag doesn't set here!\r\n    let mut state = State::from_ptr(ffi::lua_newthread(self.L));\r\n    state.reset_extra();\r\n    state\r\n  }\r\n}\r\n\r\n impl Drop for State {\r\n   fn drop(&mut self) {\r\n    if self.owned {\r\n       let _ = self.set_extra(None);\r\n       unsafe { ffi::lua_close(self.L) }\r\n     }\r\n   }\r\n```\r\n\r\nI've add three origins of a reference to a state:\r\n1. `Owned` - when new state created\r\n2. `Thread` - when new thread created (like `Owned`, but `close` never be called)\r\n3. `Restored` - when state restored from a reference\r\n\r\nI don't think we have to implement automatic cleanup by Lua's GC, because:\r\n1. It takes runtime cost\r\n2. I am convinced that if user attached some extra, they want to explore it later, and keep original `State` of thread\r\n3. It will hard to debug (simply to take panics at empty extra unwrapping)\r\n4. I'm not sure it's possible with thread object\r\n\r\nI've checked this PR. It's **100%** backward compatible."},"comment":{"url":"https://api.github.com/repos/jcmoyer/rust-lua53/issues/comments/244196635","html_url":"https://github.com/jcmoyer/rust-lua53/pull/65#issuecomment-244196635","issue_url":"https://api.github.com/repos/jcmoyer/rust-lua53/issues/65","id":244196635,"user":{"login":"sagebind","id":2192863,"avatar_url":"https://avatars.githubusercontent.com/u/2192863?v=3","gravatar_id":"","url":"https://api.github.com/users/sagebind","html_url":"https://github.com/sagebind","followers_url":"https://api.github.com/users/sagebind/followers","following_url":"https://api.github.com/users/sagebind/following{/other_user}","gists_url":"https://api.github.com/users/sagebind/gists{/gist_id}","starred_url":"https://api.github.com/users/sagebind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sagebind/subscriptions","organizations_url":"https://api.github.com/users/sagebind/orgs","repos_url":"https://api.github.com/users/sagebind/repos","events_url":"https://api.github.com/users/sagebind/events{/privacy}","received_events_url":"https://api.github.com/users/sagebind/received_events","type":"User","site_admin":false},"created_at":"2016-09-01T20:08:55Z","updated_at":"2016-09-01T20:08:55Z","body":"Even better would be to store an `Arc` within Rust, and give Lua only a weak pointer:\r\n\r\n```rust\r\npub enum State {\r\n    Owned {\r\n        L: *mut lua_State,\r\n        extra: Arc<Option<Extra>>,\r\n    },\r\n    Referenced {\r\n        L: *mut lua_State,\r\n    },\r\n}\r\n\r\n// Creates State::Owned with state.extra = Arc::new(None)\r\n// also sets Lua's extra pointer to Arc::downgrade(state.extra)\r\nlet mut state = State::new();\r\n\r\n// Gets the pointer as a Weak<Option<Extra>>\r\nstate.get_extra();\r\n\r\n// Attempts to upgrade the Weak to an Arc, and mutates its inner value\r\nstate.set_extra(Some(42));\r\n\r\n// Creates State::Referenced\r\nlet mut thread = state.new_thread();\r\n\r\n// Same behavior as Owned above\r\nthread.get_extra();\r\nthread.set_extra(Some(34));\r\n\r\n// Literally needs to do nothing about the extra, since only\r\n// one Arc<> is owned except during a set_extra() call.\r\ndrop(thread);\r\n\r\n// The Arc<> gets dropped here.\r\ndrop(state);\r\n```"}}
{"actor":{"display_login":"sagebind"}}