-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove package cycles involving lease, internal, exception, and frame #774
Conversation
Move ClientSetup and ServerSetup are closely associated with the RSocketFactory implementations. Moving them into core where they are used from breaks a package cycle between resume and internal. ClientSetup is straight-forward and folds easily into DefaultClientRSocketFactory. ConnectionUtil is closely associated with ServerSetup and has been folded into it. Signed-off-by: Rossen Stoyanchev <rstoyanchev@pivotal.io>
rsocket-core/src/main/java/io/rsocket/frame/ErrorFrameFlyweight.java
Outdated
Show resolved
Hide resolved
MissingLeaseException is moved to the lease package next to the contracts it is declared in breaking a cycle between lease and exceptions. This is an API breakage but I the impact should be minor (none? the chances of something handling it explicitly) and hence an acceptable trade-off. Signed-off-by: Rossen Stoyanchev <rstoyanchev@pivotal.io>
37b779d
to
34f6852
Compare
34f6852
to
1c2c3b6
Compare
Replaces (now deprecated) RSocketException from the exceptions package which can lead to cycles (e.g. with frame package). Other minor refinements: - error code validation - shared errorCode() implementation - error code in toString() - avoid NPE for null message, it's better to keep the original exception and null is allowed by getMessage(). Signed-off-by: Rossen Stoyanchev <rstoyanchev@pivotal.io>
1c2c3b6
to
c686d5e
Compare
*/ | ||
public RSocketException(String message, @Nullable Throwable cause) { | ||
super(Objects.requireNonNull(message, "message must not be null"), cause); | ||
super(0x00000201, message, cause); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use ErrorType.APPLICATION_ERROR
here. In any case, we have ref to ErrorType
super(0x00000201, message, cause); | |
super(ErrorType.APPLICATION_ERROR, message, cause); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed, this should be a constant. I've suggested a further change to use the constants from ErrorFrameFlyweight
.
@@ -16,41 +16,47 @@ | |||
|
|||
package io.rsocket.exceptions; | |||
|
|||
import java.util.Objects; | |||
import io.rsocket.RSocketErrorException; | |||
import reactor.util.annotation.Nullable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import reactor.util.annotation.Nullable; | |
import io.rsocket.frame.ErrorType; | |
import reactor.util.annotation.Nullable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM. Just a single minor proposal to use constant
This change deprecates ErrorType as the same constants are already declared in ErrorFrameFlyweight and there is hierarchy of exceptions with one exception per error code. Signed-off-by: Rossen Stoyanchev <rstoyanchev@pivotal.io>
a16d367
to
fa16b55
Compare
This resolves package cycles between:
lease
-internal
due toClientSetup
andServerSetup
(both related to leasing).exception
-lease
due to references betweenMissingLeaseException
andLease
.exception
-frame
due to references betweenErrorFrameFlyweight
andRSocketException
.This is mostly backwards compatible and functionality neutral except for moving
MissingLeasingException
tolease
. It's unlikely to break anything and a worthwhile trade-off.