{"action":"created","issue":{"url":"https://api.github.com/repos/apache/spark/issues/6415","labels_url":"https://api.github.com/repos/apache/spark/issues/6415/labels{/name}","comments_url":"https://api.github.com/repos/apache/spark/issues/6415/comments","events_url":"https://api.github.com/repos/apache/spark/issues/6415/events","html_url":"https://github.com/apache/spark/pull/6415","id":81059359,"number":6415,"title":"[SPARK-7873] [WIP] Fix another bug related to KryoSerializerInstance re-use in sort-shuffle","user":{"login":"JoshRosen","id":50748,"avatar_url":"https://avatars.githubusercontent.com/u/50748?v=3","gravatar_id":"","url":"https://api.github.com/users/JoshRosen","html_url":"https://github.com/JoshRosen","followers_url":"https://api.github.com/users/JoshRosen/followers","following_url":"https://api.github.com/users/JoshRosen/following{/other_user}","gists_url":"https://api.github.com/users/JoshRosen/gists{/gist_id}","starred_url":"https://api.github.com/users/JoshRosen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JoshRosen/subscriptions","organizations_url":"https://api.github.com/users/JoshRosen/orgs","repos_url":"https://api.github.com/users/JoshRosen/repos","events_url":"https://api.github.com/users/JoshRosen/events{/privacy}","received_events_url":"https://api.github.com/users/JoshRosen/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":26,"created_at":"2015-05-26T17:26:37Z","updated_at":"2015-05-26T22:02:08Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/apache/spark/pulls/6415","html_url":"https://github.com/apache/spark/pull/6415","diff_url":"https://github.com/apache/spark/pull/6415.diff","patch_url":"https://github.com/apache/spark/pull/6415.patch"},"body":"This is a somewhat obscure bug, but I think that it will seriously impact KryoSerializer users who use custom registrators which disabled auto-reset. When auto-reset is disabled, then this breaks things in some of our shuffle paths which actually end up creating multiple OutputStreams from the same shared SerializerInstance (which is unsafe). \r\n\r\nThis was introduced by a patch (SPARK-3386) which enables serializer re-use in some of the shuffle paths, since constructing new serializer instances is actually pretty costly for KryoSerializer.  We had already fixed another corner-case (SPARK-7766) bug related to this, but missed this one.  From an engineering risk management perspective, we probably should have just reverted the original serializer reuse patch and added a big cross-product-of-configurations-and-shuffle-managers test suite before attempting to fix the defects.\r\n\r\nI think that I have a pretty simple fix for this, but we still might want to consider a revert for 1.4 just to be safe.\r\n\r\nFor now, this PR adds a regression test for this bug.  I'll push followup commits to add asserts to find any other instances of this bug followed by another commit to try to fix this issue."},"comment":{"url":"https://api.github.com/repos/apache/spark/issues/comments/105679926","html_url":"https://github.com/apache/spark/pull/6415#issuecomment-105679926","issue_url":"https://api.github.com/repos/apache/spark/issues/6415","id":105679926,"user":{"login":"JoshRosen","id":50748,"avatar_url":"https://avatars.githubusercontent.com/u/50748?v=3","gravatar_id":"","url":"https://api.github.com/users/JoshRosen","html_url":"https://github.com/JoshRosen","followers_url":"https://api.github.com/users/JoshRosen/followers","following_url":"https://api.github.com/users/JoshRosen/following{/other_user}","gists_url":"https://api.github.com/users/JoshRosen/gists{/gist_id}","starred_url":"https://api.github.com/users/JoshRosen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JoshRosen/subscriptions","organizations_url":"https://api.github.com/users/JoshRosen/orgs","repos_url":"https://api.github.com/users/JoshRosen/repos","events_url":"https://api.github.com/users/JoshRosen/events{/privacy}","received_events_url":"https://api.github.com/users/JoshRosen/received_events","type":"User","site_admin":false},"created_at":"2015-05-26T22:02:08Z","updated_at":"2015-05-26T22:02:08Z","body":"Whoops, meant to post this comment earlier:\r\n\r\nI think that he root problem here is that KryoSerializerInstance can be used in a way which is unsafe even within a single thread, e.g. by creating multiple open OutputStreams from the same instance or by interleaving deserialize and deserializeStream calls.  I considered a smaller patch which adds assertions to guard against this type of \"misuse\" but abandoned that approach after I realized how convoluted the Scaladoc became.\r\n\r\nI just pushed a WIP commit which illustrates my proposed fix.  In a nutshell, I think that rather than using a single `Kryo` instance in KryoSerializerInstance, we should implement a `borrowKryo()` / `releaseKryo()` API that's backed by a \"pool\" of capacity 1.  In the common case, every call to a KryoSerializerInstnace method, do its work, then release the serializer instance back to the pool.  If the pool is empty and we need an instance, we'll allocate a new Kryo on-demand.  This makes it safe for multiple OutputStreams to be opened from the same serializer.  If we try to release a Kryo back to the pool but the pool already contains a Kryo, then we'll just discard the new Kryo.  I don't think there's a clear benefit to having a larger pool since our usages tend to fall into two cases, a) where we only create a single OutputStream and b) where we create a huge number of OutputStreams with the same lifecycle, then destroy the KryoSerializerInstance (this is what's happening in the bypassMergeSort code path that my failing test hits)."}}