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

wstransport.nim: avoid re-raising 'TransportOsError' to avoid stopping switch.accept #929

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

Ivansete-status
Copy link
Contributor

@Ivansete-status Ivansete-status commented Jul 6, 2023

Description

We found that a 'TransportOsError' can be raised by the next:

https://github.com/status-im/nim-libp2p/blob/224f92e17251464984d6906316c54d2e1108ae43/libp2p/transports/wstransport.nim#L260

 let req = await finished

Then, that 'TransportOsError' exception was re-raised in wstransport and caught by the switch.accept proc, which made the main accept loop end accepting new upcoming connections because 'TransportOsError' is 'CatchableError'.

https://github.com/status-im/nim-libp2p/blob/224f92e17251464984d6906316c54d2e1108ae43/libp2p/switch.nim#L277

Impacted component

We saw the next problem in nwaku: waku-org/nwaku#1831

Future enhancements

#927

…g `switch.accept`

We found that a 'TransportOsError' can be raised by the next:

https://github.com/status-im/nim-libp2p/blob/224f92e17251464984d6906316c54d2e1108ae43/libp2p/transports/wstransport.nim#L260

     let req = await finished

That 'TransportOsError' exception was re-raised in `wstransport` and
caught by the `switch.accept` proc, which made the main accept loop to
end accepting new upcoming connections because 'TransportOsError' is
'CatchableError'.

https://github.com/status-im/nim-libp2p/blob/224f92e17251464984d6906316c54d2e1108ae43/libp2p/switch.nim#L277
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #929 (83b40d7) into unstable (224f92e) will decrease coverage by 0.87%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable     #929      +/-   ##
============================================
- Coverage     84.32%   83.45%   -0.87%     
============================================
  Files            91       91              
  Lines         15001    15118     +117     
============================================
- Hits          12649    12617      -32     
- Misses         2352     2501     +149     
Impacted Files Coverage Δ
libp2p/transports/wstransport.nim 85.98% <0.00%> (+0.79%) ⬆️

... and 42 files with indirect coverage changes

@Ivansete-status Ivansete-status merged commit 74c402e into vacp2p:unstable Jul 7, 2023
7 of 8 checks passed
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.

2 participants