Skip to content
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

Resolve code review comments from PR 25148 #25205

Merged

Conversation

sawadood
Copy link
Contributor

@sawadood sawadood commented May 4, 2023

fixes #25201

@sawadood sawadood self-assigned this May 4, 2023
@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

#build
#spawn.fullfat.buckets=com.ibm.ws.jaxws.2.2.webcontainer_fat,com.ibm.ws.jaxws.2.2.webcontainer_fat_extended,com.ibm.ws.jaxws.cdi_fat,com.ibm.ws.jaxws.clientcontainer_fat,com.ibm.ws.jaxws.ejb_fat,com.ibm.ws.jaxws.X.wsat_fat,io.openliberty.xmlws.4.0.internal_fat,io.openliberty.ws.jaxws.global.handler.internal_fat,io.openliberty.jaxws.security_fat.ssl,io.openliberty.jaxws.security_fat.1,io.openliberty.jaxws.security_fat,com.ibm.ws.wssecurity_fat.wsscxf.1,com.ibm.ws.wssecurity_fat.wsscxf.2,com.ibm.ws.wssecurity_fat.wsscxf.3, com.ibm.ws.wssecurity_fat.wsscxf.4,com.ibm.ws.wssecurity_fat.wsscxf.saml.1,com.ibm.ws.wssecurity_fat.wsscxf.saml.2,com.ibm.ws.wssecurity_fat.wsscxf.saml.3,com.ibm.ws.wssecurity_fat.wsscxf.saml.5,com.ibm.ws.wssecurity_fat.wsscxf.saml.6,com.ibm.ws.wsat_fat.db,com.ibm.ws.wsat.common_fat,com.ibm.ws.wsat.concurrent_fat,com.ibm.ws.wsat.migration_fat.1,com.ibm.ws.wsat.migration_fat.2,com.ibm.ws.wsat.migration_fat.3,com.ibm.ws.wsat.migration_fat.4,com.ibm.ws.wsat.migration_fat.5,com.ibm.ws.wsat.recovery_fat.lps,com.ibm.ws.wsat.recovery_fat.multi.1,com.ibm.ws.wsat.recovery_fat.single.1,com.ibm.ws.wsat.recovery_fat.single.2,com.ibm.ws.jaxws.2.2.webcontainer_fat,com.ibm.ws.jaxws.2.2.webcontainer_fat_extended,com.ibm.ws.jaxws.cdi_fat,com.ibm.ws.jaxws.clientcontainer_fat,com.ibm.ws.jaxws.ejb_fat,com.ibm.ws.jaxws.X.wsat_fat,com.ibm.ws.jaxrs.2.0_fat,com.ibm.ws.jaxrs.2.0.cdi.1.2_fat,com.ibm.ws.jaxrs.2.0.client_fat,com.ibm.ws.jaxrs.2.1_fat,com.ibm.ws.jaxrs.2.1_fat_executor,com.ibm.ws.jaxrs.2.1_fat_extended,com.ibm.ws.jaxrs.2.1.cdi.2.0_fat,com.ibm.ws.jaxrs.2.1.client_fat,com.ibm.ws.jaxrs.2.1.sse_fat,com.ibm.ws.jaxrs.2.x_fat_clientProps,com.ibm.ws.microprofile.rest.client.FT_fat,com.ibm.ws.microprofile.rest.client11_fat_tck,com.ibm.ws.microprofile.rest.client12_fat_tck,com.ibm.ws.microprofile.rest.client13_fat_tck,com.ibm.ws.microprofile.rest.client14_fat_tck,com.ibm.ws.microprofile.rest.client_fat,com.ibm.ws.microprofile.rest.client_fat_tck

@LibbyBot
Copy link

LibbyBot commented May 4, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_kPx9AOrAEe2D8Kk7c5d7Ig

Target locations of links might be accessible only to IBM employees.

Copy link
Member

@jim-krueger jim-krueger left a comment

Choose a reason for hiding this comment

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

I am still concerned about the if check at line 406 in URLConnectionHTTPConduit.getInputStream(). You added a Log.fine message which indicates to me that during your testing you must have caught that exception. This concerns me because I think there is a fundamental error in the CXF code here. That if block appears to have been written for the case where connection.getErrorStream returned a non-null value (indicating that we are in an error situation). In that case it would be ok to ignore an exception from the following connection.getInputStream() call. But instead of checking that (in != null), they wrote (in == null). In that case I don't thing the exception fromgetInputStreamshould be ignored. And since you added a Log statement I'm afraid you were seeing one. Either way I think this should be fixed.

@sawadood
Copy link
Contributor Author

sawadood commented May 5, 2023

I am still concerned about the if check at line 406 in URLConnectionHTTPConduit.getInputStream(). You added a Log.fine message which indicates to me that during your testing you must have caught that exception. This concerns me because I think there is a fundamental error in the CXF code here. That if block appears to have been written for the case where connection.getErrorStream returned a non-null value (indicating that we are in an error situation). In that case it would be ok to ignore an exception from the following connection.getInputStream() call. But instead of checking that (in != null), they wrote (in == null). In that case I don't thing the exception fromgetInputStreamshould be ignored. And since you added a Log statement I'm afraid you were seeing one. Either way I think this should be fixed.

I didn't see this exception during my testing, I added a trace statement where exception was being ignored, I thought at least we should log this exception to help us debug problems. I see many instances in CXF code where exceptins are caught and ignored. I have been adding trace statements in these cases whenever I come across such code.
I think if connection.getErrorStream() returns non-null value, it indicates that we were able to successfully get InputStream from error stream. The way I read it is error has already occurred before calling connection.getErrorStream() (if (getResponseCode() >= HttpURLConnection.HTTP_BAD_REQUEST)).
If connection.getErrorStream() return null (this is what code is doing), it indicates that we were not able to get the InputStream from error stream (which indicates something went wrong in connection.getErrorStream()) and in this case the code tries to get InputStream by calling getInputStream(), instead of getErrorStream().

@jim-krueger
Copy link
Member

Thanks for the details Syed. I somehow missed that we were already in an error situation in that section of code. I'll approve now.

@LibbyBot
Copy link

LibbyBot commented May 5, 2023

The build sawadood-25205-20230504-1746
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_kPx9AOrAEe2D8Kk7c5d7Ig
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_kPx9AOrAEe2D8Kk7c5d7Ig

@sawadood sawadood force-pushed the 25201-resolve-pr-25148-comments branch from ed6250e to f9e7558 Compare May 8, 2023 13:59
@sawadood
Copy link
Contributor Author

sawadood commented May 8, 2023

#build doing another PB
#spawn.fullfat.buckets=com.ibm.ws.jaxws.2.2.webcontainer_fat,com.ibm.ws.jaxws.2.2.webcontainer_fat_extended,com.ibm.ws.jaxws.cdi_fat,com.ibm.ws.jaxws.clientcontainer_fat,com.ibm.ws.jaxws.ejb_fat,com.ibm.ws.jaxws.X.wsat_fat,io.openliberty.xmlws.4.0.internal_fat,io.openliberty.ws.jaxws.global.handler.internal_fat,io.openliberty.jaxws.security_fat.ssl,io.openliberty.jaxws.security_fat.1,io.openliberty.jaxws.security_fat,com.ibm.ws.wssecurity_fat.wsscxf.1,com.ibm.ws.wssecurity_fat.wsscxf.2,com.ibm.ws.wssecurity_fat.wsscxf.3, com.ibm.ws.wssecurity_fat.wsscxf.4,com.ibm.ws.wssecurity_fat.wsscxf.saml.1,com.ibm.ws.wssecurity_fat.wsscxf.saml.2,com.ibm.ws.wssecurity_fat.wsscxf.saml.3,com.ibm.ws.wssecurity_fat.wsscxf.saml.5,com.ibm.ws.wssecurity_fat.wsscxf.saml.6,com.ibm.ws.wsat_fat.db,com.ibm.ws.wsat.common_fat,com.ibm.ws.wsat.concurrent_fat,com.ibm.ws.wsat.migration_fat.1,com.ibm.ws.wsat.migration_fat.2,com.ibm.ws.wsat.migration_fat.3,com.ibm.ws.wsat.migration_fat.4,com.ibm.ws.wsat.migration_fat.5,com.ibm.ws.wsat.recovery_fat.lps,com.ibm.ws.wsat.recovery_fat.multi.1,com.ibm.ws.wsat.recovery_fat.single.1,com.ibm.ws.wsat.recovery_fat.single.2,com.ibm.ws.jaxws.2.2.webcontainer_fat,com.ibm.ws.jaxws.2.2.webcontainer_fat_extended,com.ibm.ws.jaxws.cdi_fat,com.ibm.ws.jaxws.clientcontainer_fat,com.ibm.ws.jaxws.ejb_fat,com.ibm.ws.jaxws.X.wsat_fat,com.ibm.ws.jaxrs.2.0_fat,com.ibm.ws.jaxrs.2.0.cdi.1.2_fat,com.ibm.ws.jaxrs.2.0.client_fat,com.ibm.ws.jaxrs.2.1_fat,com.ibm.ws.jaxrs.2.1_fat_executor,com.ibm.ws.jaxrs.2.1_fat_extended,com.ibm.ws.jaxrs.2.1.cdi.2.0_fat,com.ibm.ws.jaxrs.2.1.client_fat,com.ibm.ws.jaxrs.2.1.sse_fat,com.ibm.ws.jaxrs.2.x_fat_clientProps,com.ibm.ws.microprofile.rest.client.FT_fat,com.ibm.ws.microprofile.rest.client11_fat_tck,com.ibm.ws.microprofile.rest.client12_fat_tck,com.ibm.ws.microprofile.rest.client13_fat_tck,com.ibm.ws.microprofile.rest.client14_fat_tck,com.ibm.ws.microprofile.rest.client_fat,com.ibm.ws.microprofile.rest.client_fat_tck

@LibbyBot
Copy link

LibbyBot commented May 8, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_443SsO2mEe2D8Kk7c5d7Ig

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

LibbyBot commented May 9, 2023

The build sawadood-25205-20230508-0755
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_443SsO2mEe2D8Kk7c5d7Ig
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_443SsO2mEe2D8Kk7c5d7Ig

@sawadood
Copy link
Contributor Author

sawadood commented May 9, 2023

The personal build had 6 failures, all these failures are addressed by existing defect 295783 (https://wasrtc.hursley.ibm.com:9443/jazz/web/projects/WS-CD#action=com.ibm.team.workitem.viewWorkItem&id=295783) and are not related to this fix.

@sawadood
Copy link
Contributor Author

sawadood commented May 9, 2023

#run-libby-bot

1 similar comment
@sawadood
Copy link
Contributor Author

sawadood commented May 9, 2023

#run-libby-bot

@sawadood sawadood merged commit ffb6eee into OpenLiberty:integration May 9, 2023
@LibbyBot
Copy link

LibbyBot commented May 9, 2023

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 2 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address review comments from Pull Request 25148
4 participants