Synchronisation issue in delete-node

[originalsouth]

Please see SSC-ICT-Innovatie/nl-kat-coordination#2072.

The delete-node code

xtdb-http-multinode/src/xtdb_http_multinode/core.clj

Lines 357 to 378 in f3c5da3

	(defn- delete-node [request]
	(let [name (get-in request [:parameters :body :node])
	node (get @nodes name)]
	(if node
	(do
	;; We first remove the node so no new queries can be started
	(swap! nodes atom-delete-node name)
	(log/info "Removed node" name "from the list of nodes")
	(.close node)
	(loop []
	(let [num-queries (get @active-queries-per-node name 0)]
	(if (= num-queries 0)
	(do
	;; No queries are running anymore so we can delete the files.
	(run! io/delete-file (reverse (file-seq (io/file node-dir name))))
	(log/info "Deleted files for node" name)
	{:status 200, :body {:deleted true}})
	(do
	(log/info "Sleeping 100 ms because of" num-queries "active queries for node" name)
	(Thread/sleep 100)
	(recur))))))
	{:status 404, :body {:error "Node not found"}})))

Should probably block, or we have an await call to mitigate this.

[dekkers]

As far as I understand it this should block. Did you verify that this doesn't block? My guess is that it goes wrong because octopoes tests don't wait for the result of the delete node call.

[originalsouth]

The responsible code should be here where the verification is done here.

Here the raise_for_status is given by and ought to block:

    def raise_for_status(self):
        """Raises :class:`HTTPError`, if one occurred."""

        http_error_msg = ""
        if isinstance(self.reason, bytes):
            # We attempt to decode utf-8 first because some servers
            # choose to localize their reason strings. If the string
            # isn't utf-8, we fall back to iso-8859-1 for all other
            # encodings. (See PR #3538)
            try:
                reason = self.reason.decode("utf-8")
            except UnicodeDecodeError:
                reason = self.reason.decode("iso-8859-1")
        else:
            reason = self.reason

        if 400 <= self.status_code < 500:
            http_error_msg = (
                f"{self.status_code} Client Error: {reason} for url: {self.url}"
            )

        elif 500 <= self.status_code < 600:
            http_error_msg = (
                f"{self.status_code} Server Error: {reason} for url: {self.url}"
            )

        if http_error_msg:
            raise HTTPError(http_error_msg, response=self)

Unless pytest is doing something unexpected here I think this should all be synchronous? Perhaps @ammar92 can shed some light here?