open_tcp_listeners exception __cause__ is always an ExceptionGroup

[jakkdl]

2 years ago, 4bd09bc changed open_tcp_listeners to use ExceptionGroup instead of MultiError for the __cause__, which led to a behavior change in single-fail cases where MultiErrors would collapse into the contained exception - something that ExceptionGroup doesn't do.

I only noticed this due to some confusion on my part when interpreting codecov in #2822.

I suspect we want to stick with the current behavior and just fix the test, but splitting it out so it can be discussed.

Current open_tcp_listeners

trio/src/trio/_highlevel_open_tcp_listeners.py

Lines 166 to 173 in 60b9e14

	if unsupported_address_families and not listeners:
	msg = (
	"This system doesn't support any of the kinds of "
	"socket that that address could use"
	)
	raise OSError(errno.EAFNOSUPPORT, msg) from ExceptionGroup(
	msg, unsupported_address_families
	)

and the test

trio/src/trio/_tests/test_highlevel_open_tcp_listeners.py

Lines 326 to 335 in 60b9e14

	with pytest.raises(OSError) as exc_info:
	await open_tcp_listeners(80, host="example.org")
	assert "This system doesn't support" in str(exc_info.value)
	if isinstance(exc_info.value.__cause__, BaseExceptionGroup):
	for subexc in exc_info.value.__cause__.exceptions:
	assert "nope" in str(subexc)
	else:
	assert isinstance(exc_info.value.__cause__, OSError)
	assert "nope" in str(exc_info.value.__cause__)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9fbcb38) 99.65% compared to head (44d02ac) 99.67%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2900      +/-   ##
==========================================
+ Coverage   99.65%   99.67%   +0.01%     
==========================================
  Files         115      115              
  Lines       17310    17308       -2     
  Branches     3110     3109       -1     
==========================================
+ Hits        17251    17252       +1     
+ Misses         40       38       -2     
+ Partials       19       18       -1     

Files	Coverage Δ
...c/trio/_tests/test_highlevel_open_tcp_listeners.py	100.00% <100.00%> (+1.49%)	⬆️

[CoolCat467]

From what I understand from what you have said in your comment above, the way exceptions are handled for open_tcp_listeners is different from how the rest of Trio handles exceptions, no? If this is indeed the case, I think it would be best to make this function handle exceptions like the rest of Trio (how Trio handles things when strict_exception_groups is True I mean), unless there would be a good reason not to.

[jakkdl]

From what I understand from what you have said in your comment above, the way exceptions are handled for open_tcp_listeners is different from how the rest of Trio handles exceptions, no? If this is indeed the case, I think it would be best to make this function handle exceptions like the rest of Trio (how Trio handles things when strict_exception_groups is True I mean), unless there would be a good reason not to.

Yeah that's correct. But the current behaviour having been in place for two years with nobody complaining means that it's not especially confusing or anything, and changing the behaviour might break code that's working perfectly fine at the moment - which I think is a very good reason not to change it.

Also, re: "rest of trio" - I'm pretty sure it's only Nurseries that raise exceptiongroups and respect strict_exception_groups.
I suppose we could add a parameter strict_exception_groups to open_tcp_listeners as well, defaulting to True, in which case the only change would be to users passing strict_exception_groups=False to trio.run.. but I still don't see much to gain from that.

[CoolCat467]

Also, re: "rest of trio" - I'm pretty sure it's only Nurseries that raise exceptiongroups and respect strict_exception_groups. I suppose we could add a parameter strict_exception_groups to open_tcp_listeners as well, defaulting to True, in which case the only change would be to users passing strict_exception_groups=False to trio.run.. but I still don't see much to gain from that.

I don't think we should have a parameter for open_tcp_listeners, I think it should be fine as is as you have said. Public statement of agreement.

As for breaking compatibility, that makes sense, so I also agree there.

[Zac-HD]

I agree with the comments above that keeping the current behavior and just fixing this test is the best way forward at the moment.

[CoolCat467]

Looks good. Not sure what type of review this is, Github's UI seems to have a bug at the moment.

[jakkdl]

Looks good. Not sure what type of review this is, Github's UI seems to have a bug at the moment.

I suspect it's because the PR was already merged when you did the review.