-
Notifications
You must be signed in to change notification settings - Fork 0
[Fix] 500대 네트워크 오류 대응 #379
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
Conversation
Summary of ChangesHello @PeraSite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 애플리케이션의 네트워크 오류 처리 메커니즘을 개선하여 견고성을 높이는 것을 목표로 합니다. 서버 연결 불가 또는 응답 없음(예: UnknownHostException, SocketTimeoutException)과 같은 상황에서 사용자에게 일관된 경험을 제공하기 위해, 모든 네트워크 요청에 대해 오류 발생 시 전용 ErrorActivity로 이동하여 사용자에게 오류 메시지를 표시하도록 구현되었습니다. 이는 특정 초기 검사뿐만 아니라 앱 전반의 네트워크 실패에 대한 포괄적인 대응을 가능하게 합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
이번 PR은 OkHttp Interceptor를 사용하여 전역 네트워크 오류 처리기를 도입하여 오류 처리를 중앙 집중화하는 좋은 접근 방식을 보여줍니다. IOException이 발생하면 ErrorActivity로 이동하여 사용자에게 알립니다. 제 리뷰에는 구현을 개선하기 위한 몇 가지 제안이 포함되어 있습니다:
OkHttpClientprovider를 리팩토링하여 코드 중복을 줄입니다.ErrorActivity를 시작할 때 인텐트 플래그를 조정하여 사용자 경험이 저해되는 것을 방지합니다.ErrorActivity의 뷰 바인딩을 수정합니다.- 하드코딩된 값을 제거하고 중복 코드를 정리하여 코드 유지보수성을 향상시킵니다.
전반적으로 올바른 방향으로 변경되었으며, 몇 가지 조정을 거치면 오류 처리 메커니즘이 더욱 견고하고 사용자 친화적으로 될 것입니다.
| return chain.proceed(request) | ||
| } catch (e: IOException) { | ||
| val intent = Intent(context, ErrorActivity::class.java).apply { | ||
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK) |
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.
Intent.FLAG_ACTIVITY_CLEAR_TASK 플래그는 현재 액티비티 스택을 모두 지우고 새로운 태스크를 시작합니다. 일시적인 네트워크 오류(예: 터널 통과)가 발생했을 때 사용자가 보고 있던 화면이 모두 사라지고 오류 화면만 남게 되어 사용자 경험을 크게 해칠 수 있습니다. 이 플래그를 제거하여 사용자가 오류 확인 후 이전 화면으로 돌아갈 수 있도록 하는 것이 좋습니다. FLAG_ACTIVITY_NEW_TASK만으로도 비-액티비티 컨텍스트에서 액티비티를 시작하는 데 충분합니다.
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK) | |
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) |
| @AndroidEntryPoint | ||
| class ErrorActivity : AppCompatActivity() { | ||
|
|
||
| private lateinit var binding: ActivityIntroBinding |
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.
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.
대응햇슨
| fun provideAuthOkHttpClient( | ||
| tokenInterceptor: TokenInterceptor, | ||
| tokenAuthenticator: TokenAuthenticator | ||
| tokenAuthenticator: TokenAuthenticator, | ||
| networkErrorInterceptor: NetworkErrorInterceptor | ||
| ) = if (BuildConfig.DEBUG) { | ||
| val loggingInterceptor = HttpLoggingInterceptor() | ||
| loggingInterceptor.setLevel(HttpLoggingInterceptor.Level.BODY) | ||
|
|
||
| OkHttpClient.Builder() | ||
| .addInterceptor(networkErrorInterceptor) | ||
| .addInterceptor(loggingInterceptor) | ||
| .addInterceptor(tokenInterceptor) | ||
| .authenticator(tokenAuthenticator) | ||
| .build() | ||
| } else { | ||
| // 프로덕션 환경에서는 로깅 인터셉터를 추가하지 않음 | ||
| OkHttpClient.Builder() | ||
| .addInterceptor(networkErrorInterceptor) | ||
| .addInterceptor(tokenInterceptor) | ||
| .authenticator(tokenAuthenticator) | ||
| .build() |
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.
현재 provideAuthOkHttpClient 함수는 BuildConfig.DEBUG 값에 따라 두 개의 거의 동일한 OkHttpClient.Builder 블록을 가지고 있습니다. 로깅 인터셉터 추가 여부만 다른데, 이로 인해 코드 중복이 발생합니다. 아래와 같이 리팩토링하여 중복을 제거하고 가독성을 높일 수 있습니다.
fun provideAuthOkHttpClient(
tokenInterceptor: TokenInterceptor,
tokenAuthenticator: TokenAuthenticator,
networkErrorInterceptor: NetworkErrorInterceptor
): OkHttpClient {
val builder = OkHttpClient.Builder()
.addInterceptor(networkErrorInterceptor)
.addInterceptor(tokenInterceptor)
.authenticator(tokenAuthenticator)
if (BuildConfig.DEBUG) {
val loggingInterceptor = HttpLoggingInterceptor().apply {
level = HttpLoggingInterceptor.Level.BODY
}
builder.addInterceptor(loggingInterceptor)
}
return builder.build()
}| } catch (e: IOException) { | ||
| val intent = Intent(context, ErrorActivity::class.java).apply { | ||
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK) | ||
| putExtra("message", "서버 통신에 실패했습니다. 잠시 후 다시 시도해 주세요.") |
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.
에러 메시지와 인텐트 extra의 키("message")가 하드코딩되어 있습니다. 유지보수성과 다국어 지원을 위해 문자열은 strings.xml 리소스에 정의하고, 인텐트 키는 ErrorActivity의 companion object에 상수로 정의하여 사용하는 것을 권장합니다.
예시:
// ErrorActivity.kt
companion object {
const val EXTRA_MESSAGE = "error_message"
}
// strings.xml
<string name="network_error">서버 통신에 실패했습니다. 잠시 후 다시 시도해 주세요.</string>
// NetworkErrorInterceptor.kt
putExtra(ErrorActivity.EXTRA_MESSAGE, context.getString(R.string.network_error))|
|
||
| val baseResponse = BaseResponse<Void>( | ||
| isSuccess = false, | ||
| code = 500, // 서버 처리 오류인지 통신 불가인지 구분 |
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.
네트워크 오류를 나타내기 위해 code를 500으로 하드코딩하고 있습니다. 이 값은 IntroViewModel에서도 사용되므로, 두 파일 간의 의존성이 생깁니다. 이 값을 NetworkErrorInterceptor의 companion object에 상수로 정의하여 여러 곳에서 참조하도록 하면 코드의 일관성과 유지보수성을 높일 수 있습니다.
예시:
// NetworkErrorInterceptor.kt
companion object {
const val NETWORK_ERROR_CODE = 500
private val gson = Gson()
}
// IntroViewModel.kt
... else if (it.code != NetworkErrorInterceptor.NETWORK_ERROR_CODE) { ...| if (it.result == true) { //토큰이 있고 유효함 | ||
| _uiState.value = UiState.Success(IntroState.ValidToken) | ||
| } else { //토큰이 있어도 유효하지 않음 | ||
| } else if (it.code != 500) { // 토큰이 있어도 유효하지 않음 |
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.
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.
|
아하 제가 의도를 잘못이해하였었군요..! IOException(SocketTimeoutException, UnknownHostException)에 대한 에러 대응으로 다시 이해하고 이야기를 해볼게요. 제 짧은 견해로 보자면 일단 네트워크 오류에 전사적으로 대응하는 일반적인 방법은 ApiResult sealed class를 만드는 것입니다. 이부분에 대해서 @kangyuri1114 님과 이야기를 나누었을떄, 요러한 방법으로 진행하면 좋겠다고 했었고, 제가 아는 보편적인 방법도 해당 방법입니다. interceptor를 오버라이딩 해보셔서 알겠지만 interceptor의 역할은 api call을 요청하기 전에 낚아채서 뭔가 덧붙인 다음에 요청을 보내도록 하는 것입니다. 소프트웨어 엔지니어링에 정답은 없지만, 저는 제훈님께서 구현한 방식이 interceptor 답지 못하다고 억지스러운 구현이라는 생각이 듭니다..! 더불어 네트워크 레이어인 interceptor에서 UI를 직접적으로 호출하는것은 바람직하지 않은 것 같습니다! https://itstory1592.tistory.com/132의 맨 마지막 ApiResult을 보시면 도움이 될 것 같습니다 혹, 제가 잘못알고 있는 부분이 있다면 말씀해주시면 감사하겠습니다! |
|
저도 인터셉터를 작성하면서 아.. 이건 아닌데... 싶으면서도 처음 접해보는 라이브러리다보니 넘겼는데 정말 좋은 대안이 있네요! 말씀해주신 것처럼 UI 호출도 코드 스멜이 확실히 있구요! |
|
확인이 늦었네요..! 작성해주신 코드는 일시적인 해결에는 좋을 것 같지만, 가장 걸리는 부분은 "인터셉터에서의 intent"입니다 그리고 TimeOut, 서버가 꺼지거나 응답할 수 없는 경우, 네트워크 연결이 아예 기기에서 안될 때 발생하던 UnknownHostException, SocketTimeoutException 등등의 이 모든 예외를 "500대 서버 오류"로 한번에 보는 게 과연 적절한 예외처리일지 궁금합니다 UnknownHostException이나 SocketTimeoutException은 사실 HTTP 요청<>응답이 오가기 전에 발생하는 예외이기에 네트워크 예외에 속하기 때문에 "서버 오류"는 아닌 것 같아서요! 물론 서버가 죽으면 timeout이 나긴 하겠지만, 다른 500대의 진짜 서버 오류와 혼동될 것 같아 우려돼요 -> 여기까지는 코드 리뷰였구요! 그런데 이런 제 우려들이 위의 진님의 예시코드에서 다 해결이 될 것 같네요! 많이 고민되기 하는데.. 오래 걸리더라도 ApiResult sealed class을 도입하는 게 좋을 거 같아요. PR은 닫고 새로 작업해주심이 좋을 것 같습니다~! |
|
진님 유리님 의견 감사해요! 최대한 빠르게 ApiResult PR 만들어서 올려보겠습니당 👍 |


Summary
서버가 꺼지거나 응답할 수 없는 경우와 네트워크 연결이 아예 기기에서 안될 때 발생하던 UnknownHostException, SocketTimeoutException 대응 추가
okhttp3로 보내는 모든 서버 요청에서 IOException가 발생하면 AlertDialog를 띄우는 ErrorActivity로 이동합니다.
Describe your changes
Issue
To reviewers