NameResolutionPal.Unix enabled cancellation

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.

[gfoidl]

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate (where?)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#34633 added async support for name resolution on glibc-based Unix-distros, this PR adds cancellation.

Note

The man-page of gai_cancel states:

The request cannot be canceled if it is currently being processed; in that case, it will be handled as if gai_cancel() has never been called.

The source also shows that gai_cancel just removes the request from the queue, so the behavior might be somewhat unexpected and should be documented appropriate.

Author:	gfoidl
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[gfoidl]

See #43816 (comment)
If this is considered reliable enough, so that issue is fixed with this change.

[stephentoub]

I'll let @wfurt comment, but this seems like at some point it's going to fail spuriously in CI.

[wfurt]

It seems like the TestSettings.UncachedHost is already used twice and the second use could be cached - certainly in case of systemd resolver. If we want to improve chance of failure, I think each round should use unique name. And the test was already problematic in CI.

We could also accept the reality that cancellation is best-effort and timing sensitive and verify that if task failed, OperationCanceledException is only one reason.

We could also use SkipTestException. That would not change the test run much but we could at least see reports over time.

[gfoidl]

So we could do this strategy:

1. update the tests here to use SkipTestException, so it won't break CI but we still get some data for the cancellation-tests
2. provide a proper / more robust solution when adressing Move name resolution cancellation test into Docker #43816

[gfoidl]

62f05fe adds SkipTestException. OK as proposed in the comment before?

[wfurt]

LGTM

[karelz]

@stephentoub any additional concerns, or can we merge?

[gfoidl]

Besides the code we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix -- see top post.

[stephentoub]

we should have a look on how to document the somewhat unintuitive behavior of cancellation on unix

All attempts at cancellation are best effort, and there are many places where certain sections of execution aren't interruptible by cancellation. It's unfortunate that the support provided by glibc isn't better here, but I don't think we need to or even can get into the nitty gritty of exactly what's cancelable when. Hopefully in the future this can be improved.

[stephentoub]

I do have a more general question, though. Given the limitation, is it worth it? This change adds some complication and cost; will there generally be a payoff, or will we often be in the non-cancelable case?

[stephentoub]

No one's going to notice this, especially in CI. Could we just keep retrying until it does fail? Folks will notice hangs ;-)

[gfoidl]

Like a148365?

[gfoidl]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

[wfurt]

will we often be in the non-cancelable case?

I don't have numbers for this, but my gut feeling tells that with current implemenation in glibc, where a request is just removed from the queue, a lot of operations aren't cancelable. Especially #43816 wouldn't work ("these tests might be moved into a Docker container with a non-working DNS server that will never respond") as this will just hang.

In e.g. server environment where the queue will be flooded with requests, it's more likeley to actually cancel (= remove from the queue) a request.

TBH I don't know if it is worth it, hope that you have an answer for this 😉

To me, the value of cancelation is ability to fail lookup that takes too long - for whatever reason. That gives caller option to handle that any way they want. Whether we can actually cancel operation inside glibc is secondary IMHO. Since we register delegate, could we do that somehow?

[gfoidl]

Since we register delegate, could we do that somehow?

gai_cancel returns either EAI_CANCELLED, EAI_NOTCANCELLED, or EAI_ALLDONE.
We could return this value to

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Line 316 in a148365

Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle);

and then decide how to continue.

EAI_CANCELLED or EAI_ALLDONE are already handled by the current state of the PR.

EAI_NOTCANCELLED is the problematic case. We could return a canceled task the caller, but would need to keep the context alive in order to get no AV (I assume) if the native lookup completes anytime and invokes the callback

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 542 in a148365

callback(state->entry, ret);

which is

runtime/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

Lines 384 to 387 in a148365

	privateunsafestructGetHostEntryForNameContext
	{
	publicInterop.Sys.HostEntryResult;
	publicIntPtrState;

.

Right now I have no real idea on how to do this reliable, withou leaking memory or too much complication of the code.

[gfoidl]

Closing as per discussion in #48566

In short: getaddrinfo_a is a sub-optimal api, that doesn't perform real async name resolution. Other ways will be explored.