On Mon, May 03, 2021 at 09:14:13AM +0300, Aki Tuomi wrote:> > > On 01/05/2021 18:32 Ryan Beethe <ryan at splintermail.com> wrote: > > > > 1. Why does cmd-idle.c sometimes call client_command_free()? But > > sometimes it doesn't? > > > > For example, cmd_idle_continue() frees it in some branches but not > > others. That makes no sense to me; it seems like it should be based > > on your entrypoint (mailbox notify callback vs input callback vs > > timeout callback), not based on which branch of logic within that > > entrypoint. > > > > 2. Why does cmd-idle.c ever call client_destroy()? That seems like > > something that should be invoked only by the imap process, not by any > > command. > > > > It calls it in cmd-idle.c:idle_callback (which is a mailbox notify > > callback). It invokes it after idle_sync_now() when it detects that > > client->disconnected is set. Maybe that happens in imap_sync_init() > > or something? > > > > 3. Why does cmd-idle.c ever call client_disconnect()? That also seems > > like the responsibility of the imap process, and not any command. > > > > idle_client_input_more() detects when i_stream_read returns -1, > > meaning that the client has *already disconnected*. Then it calls > > client_disconnect(). > > > > I think this is the crazy part... the istream is effectively unique > > to the imap process, so it seems unreasonable that any command is > > responsible for cleaning it up; it should just always happen at the > > imap process level before exiting, right? > > > > IDLE cmd can be sometimes delegated to a separate worker called imap-hibernate, in which case the connection is moved to another process. This explains about all your questions.Wait, but then why does APPEND also make each of these calls? APPEND can't be hibernated, as far as I can tell?> > 4. What does client_continue_pending_input() actually do, and under > > what conditions does it need to be called? > > It means that you did not consume all the input there was.Ok, reading over the code now with this understanding makes a lot of sense, thank you.> You probably shold look some much more simple commands as insipiration. Try looking e.g. how cmd_id is implemented instead.I implemented a simpler command as well, but because it was simple I didn't have any questions :) Unfortunately I do need a long-running command more like IDLE as well. Ryan
> On 04/05/2021 16:42 Ryan Beethe <ryan at splintermail.com> wrote: > > > On Mon, May 03, 2021 at 09:14:13AM +0300, Aki Tuomi wrote: > > > > > On 01/05/2021 18:32 Ryan Beethe <ryan at splintermail.com> wrote: > > > > > > 1. Why does cmd-idle.c sometimes call client_command_free()? But > > > sometimes it doesn't? > > > > > > For example, cmd_idle_continue() frees it in some branches but not > > > others. That makes no sense to me; it seems like it should be based > > > on your entrypoint (mailbox notify callback vs input callback vs > > > timeout callback), not based on which branch of logic within that > > > entrypoint. > > > > > > 2. Why does cmd-idle.c ever call client_destroy()? That seems like > > > something that should be invoked only by the imap process, not by any > > > command. > > > > > > It calls it in cmd-idle.c:idle_callback (which is a mailbox notify > > > callback). It invokes it after idle_sync_now() when it detects that > > > client->disconnected is set. Maybe that happens in imap_sync_init() > > > or something? > > > > > > 3. Why does cmd-idle.c ever call client_disconnect()? That also seems > > > like the responsibility of the imap process, and not any command. > > > > > > idle_client_input_more() detects when i_stream_read returns -1, > > > meaning that the client has *already disconnected*. Then it calls > > > client_disconnect(). > > > > > > I think this is the crazy part... the istream is effectively unique > > > to the imap process, so it seems unreasonable that any command is > > > responsible for cleaning it up; it should just always happen at the > > > imap process level before exiting, right? > > > > > > > IDLE cmd can be sometimes delegated to a separate worker called imap-hibernate, in which case the connection is moved to another process. This explains about all your questions. > > Wait, but then why does APPEND also make each of these calls? APPEND > can't be hibernated, as far as I can tell? >Because APPEND might need to read quite a lot of data from the client.> > > 4. What does client_continue_pending_input() actually do, and under > > > what conditions does it need to be called? > > > > It means that you did not consume all the input there was. > > Ok, reading over the code now with this understanding makes a lot of > sense, thank you. > > > You probably shold look some much more simple commands as insipiration. Try looking e.g. how cmd_id is implemented instead. > > I implemented a simpler command as well, but because it was simple I > didn't have any questions :) > > Unfortunately I do need a long-running command more like IDLE as well. > > RyanWhat kind of "long running command" did you have in mind? Aki