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.

Sign up for GitHub

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

Jump to bottom

fix: make the tests pass #41

Merged
merged 11 commits into from
Jul 5, 2024
Merged

fix: make the tests pass #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e2970b4
upgrade nimcrypto
diegomrsantos Jun 26, 2024
121aa19
upgrade unittest2
diegomrsantos Jun 26, 2024
546b217
upgrade nim
diegomrsantos Jun 26, 2024
867e281
upgrade unittest2
diegomrsantos Jun 26, 2024
d003c9a
fix compilation error
diegomrsantos Jun 27, 2024
740e60d
remove nim 2 from test workflow
diegomrsantos Jun 28, 2024
1631b32
fix compilation error
diegomrsantos Jul 1, 2024
ca9a029
fix deadlock
diegomrsantos Jul 1, 2024
0550ccf
upgrade chronos
diegomrsantos Jul 1, 2024
5e5dd54
Revert "upgrade chronos"
diegomrsantos Jul 1, 2024
3c0f021
add link to PR
diegomrsantos Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest, macOS-latest, windows-latest]
nim: [1.2.18, stable]
nim: [1.6.20]
steps:
- uses: actions/checkout@v2
- uses: iffy/install-nim@v3
Expand Down
6 changes: 3 additions & 3 deletions quic.nimble
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ author = "Status Research & Development GmbH"
description = "QUIC protocol implementation"
license = "MIT"

requires "nim >= 1.2.6"
requires "nim >= 1.6.0"
requires "stew >= 0.1.0 & < 0.2.0"
requires "chronos >= 3.0.0 & < 4.0.0"
requires "nimcrypto >= 0.5.4 & < 0.6.0"
requires "nimcrypto >= 0.6.0 & < 0.7.0"
requires "ngtcp2 >= 0.32.0 & < 0.34.0"
requires "upraises >= 0.1.0 & < 0.2.0"
requires "unittest2 >= 0.0.4 & < 0.1.0"
requires "unittest2 >= 0.2.2 & < 0.3.0"
3 changes: 2 additions & 1 deletion quic/connection.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ proc startSending(connection: Connection, remote: TransportAddress) =
try:
let datagram = await connection.quic.outgoing.get()
await connection.udp.sendTo(remote, datagram.data)
except TransportError:
except TransportError as e:
connection.loop.fail(e) # This might need to be revisited, see https://github.com/status-im/nim-quic/pull/41 for more details
await connection.drop()
connection.loop = asyncLoop(send)

Expand Down
7 changes: 4 additions & 3 deletions quic/transport/ngtcp2/stream/openstate.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,10 @@ method read(state: OpenStream): Future[seq[byte]] {.async.} =
state.allowMoreIncomingBytes(result.len.uint64)

method write(state: OpenStream, bytes: seq[byte]): Future[void] =
let stream = state.stream.valueOr:
raise newException(QuicError, "stream is closed")
state.connection.send(stream.id, bytes)
# let stream = state.stream.valueOr:
# raise newException(QuicError, "stream is closed")
Comment on lines +52 to +53
Copy link

Choose a reason for hiding this comment

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

I don't get why you removed those two lines. If stream is none, it'll raise an error nevertheless. It'll just be less precise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this code, there's a compilation error: Error: (ref QuicError)(msg: "stream is closed", parent: nil) can raise an unlisted exception: ref QuicError. Would you happen to know how to fix it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

method write(state: OpenStream, bytes: seq[byte]) {.async.} =
  let stream = state.stream.valueOr:
    raise newException(QuicError, "stream is closed")
  return state.connection.send(state.stream.get.id, bytes)

fails with:
Error: undeclared identifier: 'setResult'

Copy link

@lchenut lchenut Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I dug a bit into this. Here a solution:

method write(state: OpenStream, bytes: seq[byte]): Future[void] {.raises: [QuicError].} =    
  let stream = state.stream.valueOr:    
    raise newException(QuicError, "stream is closed")    
  state.connection.send(state.stream.get.id, bytes)

With the base method modified aswell:

method write*(state: StreamState, bytes: seq[byte]) {.base, async, raises: [QuicError].} =    
  doAssert false # override this method

But I'm not satisfied at all about this. It uses an old version of chronos and thus is not up to date regarding exception. With the newer version (> 4.0 I think), you'll use {.async: (raises: [QuicError]).}.
It uses aswell upraises which I just discover, it seems to be an exception tracking for older versions of nim... Which I'm hesitant to use aswell.

But it's clearly not the goal of this PR. So I might just approve if you have nothing else to add even though it should be changed/updated in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing the base method would require changing all the other implementations, I guess. I'm not sure yet if we should raise this error. As you said, not the point of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a link to this PR

# See https://github.com/status-im/nim-quic/pull/41 for more details
state.connection.send(state.stream.get.id, bytes)

method close(state: OpenStream) {.async.} =
let stream = state.stream.valueOr: return
Expand Down
2 changes: 1 addition & 1 deletion quic/transport/packets/write.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ proc writePacketNumber(writer: var PacketWriter, datagram: var openArray[byte],

proc writePacketNumberAndPayload*(writer: var PacketWriter,
datagram: var openArray[byte]) =
let packetnumber = writer.packet.packetnumber.toMinimalBytes
let packetnumber = writer.packet.packetNumber.toMinimalBytes
let payload = writer.packet.payload
writer.writePacketLength(datagram, packetnumber.len + payload.len)
writer.writePacketNumber(datagram, packetnumber)
Expand Down
Loading