Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Add "serialVersionUID" for exceptions to avoid InvalidClassException #481

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jul 6, 2018

Which problem is this PR solving?

avoid potential InvalidClassException when perform deserialization.

Short description of the changes

add generated serialVersionUID for exceptions.

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@codecov
Copy link

codecov bot commented Jul 6, 2018

Codecov Report

Merging #481 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #481   +/-   ##
=========================================
  Coverage     88.18%   88.18%           
  Complexity      495      495           
=========================================
  Files            64       64           
  Lines          1853     1853           
  Branches        241      241           
=========================================
  Hits           1634     1634           
  Misses          141      141           
  Partials         78       78
Impacted Files Coverage Δ Complexity Δ
...exceptions/BaggageRestrictionManagerException.java 100% <ø> (ø) 2 <0> (ø) ⬇️
...al/exceptions/EmptyTracerStateStringException.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...nal/exceptions/SamplingStrategyErrorException.java 100% <ø> (ø) 2 <0> (ø) ⬇️
...xceptions/MalformedTracerStateStringException.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...nternal/exceptions/UnsupportedFormatException.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...rtracing/internal/exceptions/EmptyIpException.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...ng/internal/exceptions/NotFourOctetsException.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...ertracing/internal/exceptions/SenderException.java 57.14% <ø> (ø) 2 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4253325...aa0c639. Read the comment docs.

@pavolloffay
Copy link
Member

@quaff hi, can I ask you about your serialization use case?

Copy link
Collaborator

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@quaff
Copy link
Contributor Author

quaff commented Jul 6, 2018

@jpkrohling
Copy link
Collaborator

I don't think we'll ever need to serialize the exception (or any other class in this client, for that matter), but, having a serial version UID for Exceptions is indeed a good practice.

@quaff
Copy link
Contributor Author

quaff commented Jul 6, 2018

@jpkrohling It will used when perform RMI such as spring httpInvoker, exception will be serialized in server and deserialized in client, client rethrow it.

@pavolloffay
Copy link
Member

@quaff thanks for the link. I am not asking why we should add serialization id, but how you are using jaeger client so you need the serialization.

@quaff
Copy link
Contributor Author

quaff commented Jul 6, 2018

@pavolloffay I'm using spring httpinvoker as microservice implementation, It use java serialization by default, exceptions will propagated from server to client.

@jpkrohling
Copy link
Collaborator

It will used when perform RMI such as spring httpInvoker

Interesting. I wouldn't expect the client to be remotely called, or that an exception on the client library to propagate way back to a remote caller, but it would certainly make sense.

I'm curious though: is this an actual scenario you faced, or is it theoretical? If you faced this issue, would you mind sharing more details (source code would be even better). We could then derive more tests from it (cc @jkandasa)

@quaff
Copy link
Contributor Author

quaff commented Jul 6, 2018

The problem is not jaeger specific, but I'm using jaeger for demonstration

public interface EchoService {

	String echo(String message);

}

public class EchoServiceImpl implements EchoService {

	@Override
	public String echo(String message) {
		try (Scope scope = GlobalTracer.get().scopeManager().activate(GlobalTracer.get().buildSpan("echo").start(),
				true)) {
			return message;
		}
	}

}
@Component
public class Test {

	@Autowired
	private EchoService echoService;

	public void test() {
		echoService.echo("something"); // exception from server will be thrown
	}

}

@quaff
Copy link
Contributor Author

quaff commented Jul 6, 2018

@jpkrohling It's not an actual scenario, I found it because eclipse reports warning, but I have faced same issue caused by missing serialVersionUID.

@ghost ghost assigned jpkrohling Jul 9, 2018
@ghost ghost added the review label Jul 9, 2018
@jpkrohling jpkrohling assigned quaff and unassigned jpkrohling Jul 9, 2018
@jpkrohling jpkrohling merged commit af9efcf into jaegertracing:master Jul 9, 2018
@ghost ghost removed the review label Jul 9, 2018
@jpkrohling
Copy link
Collaborator

Thanks @quaff !

@quaff quaff deleted the patch-3 branch July 12, 2018 07:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants