{"action":"closed","issue":{"url":"https://api.github.com/repos/emersion/go-imap/issues/230","repository_url":"https://api.github.com/repos/emersion/go-imap","labels_url":"https://api.github.com/repos/emersion/go-imap/issues/230/labels{/name}","comments_url":"https://api.github.com/repos/emersion/go-imap/issues/230/comments","events_url":"https://api.github.com/repos/emersion/go-imap/issues/230/events","html_url":"https://github.com/emersion/go-imap/issues/230","id":424460504,"node_id":"MDU6SXNzdWU0MjQ0NjA1MDQ=","number":230,"title":"defer closing channel in fetch should be earlier","user":{"login":"wucdbm","id":1559022,"node_id":"MDQ6VXNlcjE1NTkwMjI=","avatar_url":"https://avatars0.githubusercontent.com/u/1559022?v=4","gravatar_id":"","url":"https://api.github.com/users/wucdbm","html_url":"https://github.com/wucdbm","followers_url":"https://api.github.com/users/wucdbm/followers","following_url":"https://api.github.com/users/wucdbm/following{/other_user}","gists_url":"https://api.github.com/users/wucdbm/gists{/gist_id}","starred_url":"https://api.github.com/users/wucdbm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wucdbm/subscriptions","organizations_url":"https://api.github.com/users/wucdbm/orgs","repos_url":"https://api.github.com/users/wucdbm/repos","events_url":"https://api.github.com/users/wucdbm/events{/privacy}","received_events_url":"https://api.github.com/users/wucdbm/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2019-03-23T04:04:33Z","updated_at":"2019-04-09T05:52:24Z","closed_at":"2019-04-09T05:52:24Z","author_association":"NONE","body":"Hey guys,\r\n\r\nAs I'm quite new to golang, this took me a while to figure out. The problem is in package client, cmd_selected.go\r\n\r\nBasically, I'm using UidFetch, passing in a channel to accept `*imap.Message`, then transforming them to my own model as I need to, and in my app, returning `chan *MyOwnMessageModel, chan error`\r\n\r\nThe problem is, I'd like to be able to read *multiple* errors from the error channel as there could be errors when parsing messages, not just client-related. So basically, when an ErrNoMailboxSelected error is returned, my channel passed to UidFetch is not being closed. I'm relying on the fact it'll be closed once the fetch is completed (either with an error or successfully), where I could also close my `out` channel.\r\n\r\nI could for example introduce two error channels, read via UidFetch in goroutine, write the UidFetch error in a separate channel, then read from it in the goroutine that transforms the data read and then re-write the error in another channel, check what it is and close the UidFetch channel myself, but that seems like too much complexity given that closing this channel could be moved 3 lines up. Please note I'm returning the same channel created in my `Reader`'s UidFetch to which I'm writing the error returned by IMAP Client's UidFetch.\r\n\r\nOn a second thought, I guess I could make it simpler by getting the error returned by go-imap/client.Client.UidFetch() and assert its type to be `ErrNoMailboxSelected`, then close the `chan *imap.Message` channel myself, but that doesn't seem like a very clean usage and it was somewhat counter-intuitive to me, in this case, leaving me to wonder what's going on and where exactly my bug could be.\r\n\r\nIs there any particular reason why deferred closing is below the `if c.State() != imap.SelectedState {` if statement, and is it possible to move it up and always close the `ch` channel?\r\n\r\n```\r\nfunc (c *Client) fetch(uid bool, seqset *imap.SeqSet, items []imap.FetchItem, ch chan *imap.Message) error {\r\n\tif c.State() != imap.SelectedState {\r\n\t\treturn ErrNoMailboxSelected\r\n\t}\r\n\r\n\tdefer close(ch)\r\n\r\n\tvar cmd imap.Commander\r\n\tcmd = &commands.Fetch{\r\n\t\tSeqSet: seqset,\r\n\t\tItems:  items,\r\n\t}\r\n\tif uid {\r\n\t\tcmd = &commands.Uid{Cmd: cmd}\r\n\t}\r\n\r\n\tres := &responses.Fetch{Messages: ch}\r\n\r\n\tstatus, err := c.execute(cmd, res)\r\n\tif err != nil {\r\n\t\treturn err\r\n\t}\r\n\treturn status.Err()\r\n}\r\n```\r\nHere's my code, to give you an idea what I'm trying to accomplish\r\n```\r\nfunc (r *Reader) UidFetch(seqSet *imap.SeqSet) (chan *imap.Message, chan error) {\r\n\tmessages := make(chan *imap.Message, 5)\r\n\terrChan := make(chan error, 1)\r\n\tgo func() {\r\n\t\tsection := &imap.BodySectionName{}\r\n\t\terrChan <- r.IMAP.UidFetch(seqSet, []imap.FetchItem{section.FetchItem(), imap.FetchEnvelope, imap.FetchUid}, messages)\r\n\t}()\r\n\r\n\treturn messages, errChan\r\n}\r\n\r\nfunc (r *Reader) ReadMessages(seqSet *imap.SeqSet) (chan *Message, chan error) {\r\n\tin, errChan := r.UidFetch(seqSet)\r\n\r\n\tout := make(chan *Message, 5)\r\n\r\n\tgo func() {\r\n\t\tfor {\r\n\t\t\tm, more := <-in\r\n\t\t\tif !more {\r\n\t\t\t\tclose(out)\r\n\t\t\t\treturn\r\n\t\t\t}\r\n\r\n\t\t\tmessage, err := convertImapMessageToMessage(m)\r\n\r\n\t\t\tif err != nil {\r\n\t\t\t\terrChan <- err\r\n\t\t\t} else {\r\n\t\t\t\tout <- message\r\n\t\t\t}\r\n\t\t}\r\n\t}()\r\n\r\n\treturn out, errChan\r\n}\r\n```\r\n\r\nReading is done like so\r\n\r\n```\r\nread:\r\n\t\t\t\tfor {\r\n\t\t\t\t\tselect {\r\n\t\t\t\t\tcase err := <-errorChan:\r\n\t\t\t\t\t\tif err != nil {\r\n\t\t\t\t\t\t\tlog.Println(fmt.Sprintf(\"Error reading messages: %s\", err.Error()))\r\n\t\t\t\t\t\t}\r\n\t\t\t\t\tcase m, more := <-messages:\r\n\t\t\t\t\t\tif !more {\r\n\t\t\t\t\t\t\tbreak read\r\n\t\t\t\t\t\t}\r\n\r\n\t\t\t\t\t\tr.Write(m) // for the record, this writes them to a separate app, this service is just a supplement that's just way more stable written in Go than in PHP\r\n\t\t\t\t\t}\r\n\t\t\t\t}\r\n```\r\n\r\nPS Just figured that maybe I should probably close the `errChan` in addition to `out` when `in` is drained? Guess that won't fix the issue at hand though.\r\n\r\nPPS I've already read through #131 and tried v1 beta, but mentioned code appears to be the same.\r\n"}}
{"actor":{"display_login":"emersion"}}