MDNS.queryService() seems to be not flushing buffer #3068

kdsoo · 2017-03-21T13:47:12Z

I'm using huzzah esp8266 board with esp8266 arduino version 2.3.0

I made a http endpoint to test mdns service query and here is the piece of code:
`
void queryMDNS() {

String s;

Serial.println("Sending mDNS query");

Serial.print(server.argName(0));

Serial.print(":");

Serial.println(server.arg(0));

String service = server.arg(0);

s = "<!DOCTYPE HTML>\r\n<html><body>node(WiFi) ";

int n = MDNS.queryService(service, "tcp"); // Send out query for esp tcp services

Serial.println("mDNS query done");

if (n == 0) {

	Serial.println("no services found");

	s += "<p>No service found</p></body></html>";

} else {

	Serial.print(n);

	Serial.println(" service(s) found");

	s += "<p>Service found</p>";

	for (int i = 0; i < n; ++i) {

		// Print details for each service found

		Serial.print(i + 1);

		Serial.print(": ");

		Serial.print(MDNS.hostname(i));

		s += "<p>";

		s += i + 1;

		s += " ";

		s += MDNS.hostname(i);

		Serial.print(" (");

		Serial.print(MDNS.IP(i));

		Serial.print(":");

		Serial.print(MDNS.port(i));

		Serial.println(")");

		s += " (";

		s += MDNS.IP(i);

		s += ":";

		s += MDNS.port(i);

		s += ")</p>";

	}

}

server.send(200, "text/html", s);

}

`

To invoke this query I access to the url like follows:

http://ESP_IP_ADDR/mdns?service=SERVICENAME

and this query returns hostname and port.

But the problem is when the query fails.
Even the query failed, MDNS.queryService and MDNS.hostname() , IP(), port()...
they still return previously discovered servie host information.
And after the query gets the right response with service information, those hostname(), IP(), port()... are refreshed with proper answer right away.

The text was updated successfully, but these errors were encountered:

kdsoo · 2017-03-23T17:13:06Z

I made a patch #3076 which resolves this problem and hopefully to be merged.
Since the problem has fixed so far and no collateral bug appears.

devyte · 2017-10-01T01:57:59Z

@kdsoo your patch may cause a mem leak on _query. You should not NULL the pointers blindly. Instead, you should e.g.: zero out the first byte in case of a string.

joernweissenborn · 2017-11-22T23:27:02Z

Wouldn't it be enough to add

_answers = 0;

here?

EDIT: I quickly tested it and it does seem to fix the issue.

devyte · 2017-11-23T03:06:06Z

@joernweissenborn at first glance, I think that would also cause a memleak, because _answers is a ptr to the head of a linked list.
I believe that what needs to happen is that the linked list needs to be freed and cleared at the start of the query call. However, close inspection of the code logic is needed to determine if that by itself is correct.

As a side note to whoever looks at this, the String objects in the linked lists should be reserved for size before assignment.

joernweissenborn · 2017-11-24T00:32:02Z

Thanks for the hint. I think I understand the problem.

One thing which puzzles me is, that when you have 2 or more responders for a query and all of them go away, you are left of with one response.

So the list of answers get flushed, but there seams to be one element remaining. Since you stated, _answers is a linked list, it seams like the problem is cleaning up the last or first item in the list. I haven't dived deeper into code yet.

Background: For me it is a crucial bug since I I want my stuff to be self healing. This means, that whenever there is a connection failure, I restart the the cycle (Lookup -> TcpConnect). I want to avoid the prone to fail TcpConnect on the old record.

joernweissenborn · 2017-11-24T18:28:45Z

I looked deeper into it. It seams like the answers are supposed to be cleaned up by _parsePacket. This functions is bound to _conn.onRx. So if there is no response, _parsePacket doesn't get invoked and thus the list is not cleaned.

Question: Is there any reason not to perform the cleanup in queryService so the answers will be cleared no matter whether there is a response or not?

joernweissenborn · 2017-11-29T04:13:57Z

I dug deeper into problem:

At the moment, when there is an Rx event, _parsePacket is called as callback. Within the function, there is check if the _newQuery flag is set. If so, it flushes the answer list. There is only one place in the code which actually sets this flag, the queryService function. It sets it right before emitting the query. I guess the main reason to put it there is to get safety in the case a Rx interrupt comes in during the call.

I guess my PR would introduce undefined behavior in case we have an Rx while cleanup. I closed it.

Another possible fix of this I thought about is rather simple: in the _getNumAnswers function, one could could simply check the if _newQuery is still set (the first response will unset it) and return 0 in that case.

devyte · 2017-11-29T04:17:16Z

Feel free to propose a new solution. Unfortunately, I can't dig into it myself right now.

joernweissenborn · 2017-11-29T04:25:19Z

My idea would be to add:

if (_newQuery) {
  return 0;
}

here.

I am coming from go, so I am not veryfamiliar with manual mem managment. But fo me it seams to be a good solution, since it does not touch the current logic and just reports the users that there have been no responses. The old list will still be cleaned up upon next successfull query (untouched otherwise).

devyte · 2018-12-05T20:05:28Z

Closing in view of #5442 with a full rewrite of mdns.

joernweissenborn mentioned this issue Nov 24, 2017

ESP8266mDNS: Fix flushing answers when there is no query response. #3878

Closed

devyte self-assigned this Feb 8, 2018

devyte added this to the 2.5.0 milestone Feb 8, 2018

devyte added type: bug component: libraries component: network labels Feb 8, 2018

devyte closed this as completed Dec 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDNS.queryService() seems to be not flushing buffer #3068

MDNS.queryService() seems to be not flushing buffer #3068

kdsoo commented Mar 21, 2017 •

edited

Loading

kdsoo commented Mar 23, 2017

devyte commented Oct 1, 2017

joernweissenborn commented Nov 22, 2017 •

edited

Loading

devyte commented Nov 23, 2017

joernweissenborn commented Nov 24, 2017 •

edited

Loading

joernweissenborn commented Nov 24, 2017

joernweissenborn commented Nov 29, 2017 •

edited

Loading

devyte commented Nov 29, 2017

joernweissenborn commented Nov 29, 2017 •

edited

Loading

devyte commented Dec 5, 2018

MDNS.queryService() seems to be not flushing buffer #3068

MDNS.queryService() seems to be not flushing buffer #3068

Comments

kdsoo commented Mar 21, 2017 • edited Loading

kdsoo commented Mar 23, 2017

devyte commented Oct 1, 2017

joernweissenborn commented Nov 22, 2017 • edited Loading

devyte commented Nov 23, 2017

joernweissenborn commented Nov 24, 2017 • edited Loading

joernweissenborn commented Nov 24, 2017

joernweissenborn commented Nov 29, 2017 • edited Loading

devyte commented Nov 29, 2017

joernweissenborn commented Nov 29, 2017 • edited Loading

devyte commented Dec 5, 2018

kdsoo commented Mar 21, 2017 •

edited

Loading

joernweissenborn commented Nov 22, 2017 •

edited

Loading

joernweissenborn commented Nov 24, 2017 •

edited

Loading

joernweissenborn commented Nov 29, 2017 •

edited

Loading

joernweissenborn commented Nov 29, 2017 •

edited

Loading