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

Improve JsonDecoder StreamWriter dispose #1448

Merged
merged 7 commits into from
Jul 9, 2021
Merged

Conversation

mregen
Copy link
Contributor

@mregen mregen commented Jul 7, 2021

@mregen mregen requested a review from koepalex July 7, 2021 15:07
@mregen mregen marked this pull request as draft July 7, 2021 15:07
Stack/Opc.Ua.Core/Types/Encoders/JsonEncoder.cs Outdated Show resolved Hide resolved
{
m_writer?.Dispose();
m_writer = null;
m_destination.Dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary? m_writer?.Dispose() should internally dispose the memory stream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I wasn't sure...

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #1448 (45695af) into master (08164fe) will increase coverage by 0.01%.
The diff coverage is 84.48%.

❗ Current head 45695af differs from pull request most recent head 6c7a801. Consider uploading reports for the commit 6c7a801 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
+ Coverage   51.61%   51.62%   +0.01%     
==========================================
  Files         305      305              
  Lines       58590    58598       +8     
==========================================
+ Hits        30239    30253      +14     
+ Misses      28351    28345       -6     
Impacted Files Coverage Δ
...es/Opc.Ua.PubSub/Transport/MqttPubSubConnection.cs 27.96% <0.00%> (ø)
...ies/Opc.Ua.PubSub/Transport/UdpPubSubConnection.cs 27.95% <0.00%> (ø)
Libraries/Opc.Ua.PubSub/UaNetworkMessage.cs 80.00% <ø> (ø)
Stack/Opc.Ua.Core/Types/Encoders/JsonDecoder.cs 79.74% <0.00%> (ø)
Stack/Opc.Ua.Core/Types/Encoders/JsonEncoder.cs 83.51% <88.57%> (+0.32%) ⬆️
...aries/Opc.Ua.PubSub/Encoding/JsonNetworkMessage.cs 92.89% <100.00%> (-0.21%) ⬇️
...aries/Opc.Ua.PubSub/Encoding/UadpNetworkMessage.cs 87.58% <100.00%> (-0.09%) ⬇️
Libraries/Opc.Ua.PubSub/UaPubSubConnection.cs 45.19% <100.00%> (+0.53%) ⬆️
Stack/Opc.Ua.Core/Types/Encoders/BinaryEncoder.cs 78.00% <100.00%> (+0.04%) ⬆️
Stack/Opc.Ua.Core/Types/Encoders/XmlDecoder.cs 73.77% <100.00%> (ø)
... and 4 more

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 08164fe...6c7a801. Read the comment docs.

@mregen
Copy link
Contributor Author

mregen commented Jul 8, 2021

After discussion:

  • to not break existing code, let CloseXXX still dispose the writer
  • add constructor with new signature to use stream and bool leaveOpen
  • unify stream pattern across all encoders

@lgtm-com
Copy link

lgtm-com bot commented Jul 8, 2021

This pull request introduces 3 alerts when merging 2c88916 into b9ad021 - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

@mregen mregen requested a review from AlinMoldovean July 8, 2021 12:17
@mregen mregen marked this pull request as ready for review July 8, 2021 12:44
@lgtm-com
Copy link

lgtm-com bot commented Jul 8, 2021

This pull request introduces 3 alerts and fixes 2 when merging 05ab8a5 into b9ad021 - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Missing Dispose call on local IDisposable

@lgtm-com
Copy link

lgtm-com bot commented Jul 8, 2021

This pull request introduces 3 alerts and fixes 2 when merging e3d8c33 into 08164fe - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Missing Dispose call on local IDisposable

@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2021

This pull request introduces 3 alerts and fixes 2 when merging 45695af into 08164fe - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Missing Dispose call on local IDisposable

@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2021

This pull request introduces 3 alerts and fixes 2 when merging 6c7a801 into 08164fe - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Missing Dispose call on local IDisposable

@mregen mregen merged commit c435e58 into master Jul 9, 2021
@mregen mregen deleted the jsonencoder_dispose branch July 9, 2021 05:44
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.

JSONEncoder should use Stream, support leaveOpen on close
3 participants