exception: make Ipv6Instance and Ipv4Instance not throw and remove some try catch pattern

[chaoqin-li1123]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:This is part of the effort to remove C++ exception from data plane by adding assertion that the code is executed in main thread when an exception is caught.(#14320) By making the constructor of instances(PipeInstance, Ipv6Instance, Ipv4Instance) no throw, remove some try catch code from envoy.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

[jmarantz]

looks great, thank! Mostly just don't want to drop error message detail here.

[chaoqin-li1123]

We need a template method to return different type of instance instead of a ptr to their base class because some Instance has non-virtual method. I would like to name these two methods createInstancePtr and createInstance.

[jmarantz]

I'm not sure what state this is in. Can you ping me directly when this is ready for another round of deep review?

[jmarantz]

Per f2f discussion, in cases where there are a large number of references to raw constructors in the codebase, a better way to go might be:

• Put the constructor logic in a new constructor that takes an absl::Status& as its first arg, and returns its error status that way
• Make the existing constructor somehow call that one, capturing the Status and throwing if there is a failure, propagating the message. Independent of whether it fails, assert isMainThread

This way most of the call-sites don't need to change, but we are verifying you can only call them in the main thread context.

I think the tricky part is the pattern of having the throwing constructor calling the non-throwing one as it won't have a good place to store the Status. So factoring out the logic might be a little annoying. One possible remedy is to put all the logic in an 'init' method that returns the Status, so the two calls are:

// throwing-constructor:
Foo::Foo(ags...) {
  ASSERT(Thread::isMainThread());
  absl::Status status = init(args);
  if (!status.ok()) {
    throw EnvoyException(status.message());
  }
}

// non-throwing-constructor
Foo::Foo(absl::Status& status, args...) {
  status = init(args);
}

The drawback here is that you won't be able to use contructor-initializers which means none of the member vars can be const. Not sure how important that is.

[chaoqin-li1123]

Per f2f discussion, in cases where there are a large number of references to raw constructors in the codebase, a better way to go might be:

• Put the constructor logic in a new constructor that takes an absl::Status& as its first arg, and returns its error status that way
• Make the existing constructor somehow call that one, capturing the Status and throwing if there is a failure, propagating the message. Independent of whether it fails, assert isMainThread

This way most of the call-sites don't need to change, but we are verifying you can only call them in the main thread context.

I think the tricky part is the pattern of having the throwing constructor calling the non-throwing one as it won't have a good place to store the Status. So factoring out the logic might be a little annoying. One possible remedy is to put all the logic in an 'init' method that returns the Status, so the two calls are:

// throwing-constructor:
Foo::Foo(ags...) {
  ASSERT(Thread::isMainThread());
  absl::Status status = init(args);
  if (!status.ok()) {
    throw EnvoyException(status.message());
  }
}

// non-throwing-constructor
Foo::Foo(absl::Status& status, args...) {
  status = init(args);
}

The drawback here is that you won't be able to use contructor-initializers which means none of the member vars can be const. Not sure how important that is.

I think it is fine to change all the caller, since most of the code that call instance constructor directly are inside a test file instead of source code, it is safe to replace them all.

[jmarantz]

I think you are basically on the right track but need a few passes to clean up, factor out common patterns like "OrDie" correctly, avoid swallowing errors where they weren't swallowed before.

[jmarantz]

previously an error here would have resulted in a Throw, right? Should we return the StatusOr here so that callers can find the error message where they previously had caught an error and logged it in context?

In general you should make sure your changes don't drop information that may be needed when debugging a crash with logs.

[chaoqin-li1123]

Change the implementation of addressFromSockAddr(), parseInternetAddressNoThrow() and getAddressWithPort()
to use non throw version.

[jmarantz]

/wait

[jmarantz]

Thanks so much, Chaoqin! This looks like a great improvement and a major step in the right direction.

/assign-from @envoyproxy/senior-maintainers

[repokitteh-read-only]

@envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #16122 (review) was submitted by @jmarantz.

see: more, trace.

[htuch]

Since this is used in multiple .cc, it's basically a public interface, can you please document it better? Thanks.

[htuch]

Why are we no longer returning shared ptr but instead unique ptr?

[jmarantz]

Yeah I think I never fully understood the answer to this.

[chaoqin-li1123]

Returning a unique_ptr from factory is a good practice because it makes the interface more general. unique_ptr can be implicitly cast to shared_ptr but not vice versa. So return a unique_ptr is equivalent to return a unique_ptr or shared_ptr.

[jmarantz]

I think it's probably better not to change this interface in this PR just because it's better practice. A separate PR can be filed for that.

Large PRs that change semantics should minimize other changes, especially to interfaces.

[htuch]

Maybe I'm missing something here, but it seems we're not improving anything if we're still either throwing or dying on the working thread?

Can you maybe explain the context of this PR so I can understand scope better. I.e. what's the long term game plan here.

[jmarantz]

/wait

[chaoqin-li1123]

This was an attempt to remove 2 TRY_NEEDS_AUDIT at first. But the try in the dns_impl.cc can't be actually removed because of fuzz test issue, so we only remove the TRY_NEEDS_AUDIT in addressFromSockAddrOrDie. I think eventually we want to clean up all the TRY_NEEDS_AUDIT by either whitelisting them or refactoring them.

[chaoqin-li1123]

the non-throw constructor of address instance allow creating an instance in worker thread without throwing, though I can't find any code where these error status can be handled and then return back to normal execution.

[chaoqin-li1123]

From my point of view, we are trying to clean up the TRY_NEEDS_AUDIT.

[htuch]

Why can't initHelper() or some variant be used here?

[htuch]

Friendly ping

[chaoqin-li1123]

because initHelper() is used to remove duplicate code in throw and not throw version of ctors, we don't have a not throw version of this ctor.

[htuch]

I'm not spotting where this new constructor variant is used, can you point out?

[chaoqin-li1123]

Address::InstanceFactory::createInstancePtrAddress::PipeInstance(sun, ss_len); In addressFromSockAddr()

[htuch]

LGTM as a step forward, I guess we're not fundamentally changing exception safety yet here.

[htuch]

Nit: seems unrelated, but since it's a minor comment fix don't feel that strongly.