Skip to content

Conversation

@Niels-Erik
Copy link
Contributor

@Niels-Erik Niels-Erik commented Feb 20, 2025

Pull request for adding location data / line number to faults (see #514)

I've combed over every place that creates a fault and all the places that I could find that forwards them.

I've tested the following manually and seen that they report line numbers(nearly correctly see #582):

  • throwing faults
  • causing faults from arithmetic (divide by zero and ++ operator on non numbers)
  • faults coming from calls (examples call@service()() )
  • faults forwarded by forward statement
  • faults send over Local and not (used sodep)
  • TypeMismatch that are thrown as fault have context (some TypeMismatch arning are logged not using faults)

There exists some exception that can come up in ScopeProcess and OutputPort which I've not found ways to test.

updated logUnhandledFault to also print the location.
added context to allclasses that throws faults and were straight forward to add them to.
added some TODOs for:
TypeMismatch logging in RequestResponseProcess does not print location.
ScopeProcess does not add context when an abitrary Excpetion is thrown.
OutputPort does not add context for when the following exceptions are turned into Faults: ( ExecutionException | InterruptedException | IOException  )
LocalCommChannel needs to handle sending faults differently, as to not overwrite location data.

All places that creates FaultException has been reviewed, and there should be no missing changes or TODOs.
@Niels-Erik Niels-Erik marked this pull request as ready for review February 22, 2025 12:22
@Niels-Erik Niels-Erik changed the title Add location data to faults Add location data / line numbers to faults Feb 24, 2025
@fmontesi
Copy link
Member

Nice work! Two questions:

  • Why addContext, when it seems like a setter method? Shouldn't it be called setContext?
  • I'm wondering about that CommMessage::faultClone method. Can we avoid introducing it by calling setContext on the message's fault maybe?

I'm slightly concerned about making FaultException mutable with setContext. We could consider a withContext method that returns a new FaultException with the context set in, I guess, but could be overengineering. Any thoughts?

@Niels-Erik
Copy link
Contributor Author

  • Why addContext, when it seems like a setter method? Shouldn't it be called setContext?

Yes it should, it was a leftover from when I tested with a list of all contexts that the fault would could get, I'll change it.

  • I'm wondering about that CommMessage::faultClone method. Can we avoid introducing it by calling setContext on the message's fault maybe?

Sadly not, this is done to avoid an embedded service show the same fault line as the embedder. Setting the context to null in fault will still cause the same issue. The issue being that the embedded and the embedder points to the same fault.
I did think about using a clone of the fault when making the CommMessage, but since it is only a problem for local communication I opted for the CommMessage::faultClone which is only used one specific place.

I'm slightly concerned about making FaultException mutable with setContext. We could consider a withContext method that returns a new FaultException with the context set in, I guess, but could be overengineering. Any thoughts?

I feel this would be overengineering, as I see no reason to create a new object when we want to add the context, it could be useful if we opted to handle the local communication issue outside of LocalCommChannel, but again I think this would break some rules in SOLID.

… new object, making FaultExceptions immutable again
@Niels-Erik
Copy link
Contributor Author

FaultException are now immutable again by using withContext, this corrrects everything mentioned(#584 (comment)).

However there is now more overhead, as FaultExceptions are created just to create a nearly identical FaultException.
Creating more constructors would fix the overhead, but introduce twice the amount of constructors, should this be done?

@fmontesi
Copy link
Member

How many constructors are we talking about? If it's like adding 2 to 4 constructors, I'd do it.

@Niels-Erik
Copy link
Contributor Author

Niels-Erik commented Feb 27, 2025

How many constructors are we talking about? If it's like adding 2 to 4 constructors, I'd do it.

Short of it 6 new constructors.

There exists 6 which all potentially would be duplicated, and from my quick look through it was 5 or 6 needed, and then nearly all withContent methods could be removed, if not all with a bit of refactoring.

…ces of the use of withContext to use a contructor instead, when available.
@fmontesi fmontesi merged commit 2285b4c into jolie:master Mar 3, 2025
6 checks passed
@Niels-Erik Niels-Erik deleted the Faultlines branch September 14, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants