• Ondřej Surý's avatar
    Run .closehandle_cb asynchrounosly in nmhandle_detach_cb() · bfa4b9c1
    Ondřej Surý authored and Michał Kępień's avatar Michał Kępień committed
    When sock->closehandle_cb is set, we need to run nmhandle_detach_cb()
    asynchronously to ensure correct order of multiple packets processing in
    the isc__nm_process_sock_buffer().  When not run asynchronously, it
    would cause:
    
      a) out-of-order processing of the return codes from processbuffer();
    
      b) stack growth because the next TCP DNS message read callback will
         be called from within the current TCP DNS message read callback.
    
    The sock->closehandle_cb is set to isc__nm_resume_processing() for TCP
    sockets which calls isc__nm_process_sock_buffer().  If the read callback
    (called from isc__nm_process_sock_buffer()->processbuffer()) doesn't
    attach to the nmhandle (f.e. because it wants to drop the processing or
    we send the response directly via uv_try_write()), the
    isc__nm_resume_processing() (via .closehandle_cb) would call
    isc__nm_process_sock_buffer() recursively.
    
    The below shortened code path shows how the stack can grow:
    
     1: ns__client_request(handle, ...);
     2: isc_nm_tcpdns_sequential(handle);
     3: ns_query_start(client, handle);
     4:   query_lookup(qctx);
     5:     query_send(qctcx->client);
     6:       isc__nmhandle_detach(&client->reqhandle);
     7:         nmhandle_detach_cb(&handle);
     8:           sock->closehandle_cb(sock); // isc__nm_resume_processing
     9:             isc__nm_process_sock_buffer(sock);
    10:               processbuffer(sock); // isc__nm_tcpdns_processbuffer
    11:                 isc_nmhandle_attach(req->handle, &handle);
    12:                 isc__nm_readcb(sock, req, ISC_R_SUCCESS);
    13:                   isc__nm_async_readcb(NULL, ...);
    14:                     uvreq->cb.recv(...); // ns__client_request
    
    Instead, if 'sock->closehandle_cb' is set, we need to run detach the
    handle asynchroniously in 'isc__nmhandle_detach', so that on line 8 in
    the code flow above does not start this recursion. This ensures the
    correct order when processing multiple packets in the function
    'isc__nm_process_sock_buffer()' and prevents the stack growth.
    
    When not run asynchronously, the out-of-order processing leaves the
    first TCP socket open until all requests on the stream have been
    processed.
    
    If the pipelining is disabled on the TCP via `keep-response-order`
    configuration option, named would keep the first socket in lingering
    CLOSE_WAIT state when the client sends an incomplete packet and then
    closes the connection from the client side.
    bfa4b9c1