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

Various error handling and processing fixes #228

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Various error handling and processing fixes #228

merged 4 commits into from
Oct 22, 2024

Conversation

arnetheduck
Copy link
Member

@arnetheduck arnetheduck commented Oct 19, 2024

  • remove redundant gcsafe/raises
  • rework async raises to chronos 4.0 where this was not yet done
  • streamline logging between http/socket/ws
    • don't log error when raising exceptions (whoever handles should log)
    • debug-log requests in all variants of server and client
  • unify ipv4/ipv6 address resolution, with preference for ipv6
  • fix server start so that it consistently raises only when no addresses could be bound
  • also return response from serveHTTP when using chunked encoding

* remove redundant gcsafe/raises
* rework async raises to chronos 4.0 where this was not yet done
* streamline logging between http/socket/ws
  * don't log error when raising exceptions (whoever handles should log)
  * debug-log requests in all variants of server and client
* unify ipv4/ipv6 address resolution, with preference for ipv6
* fix server start so that it consistently raises only when no addresses
could be bound
Makes update less intrusive, even if it doesn't make a practical
difference
@tersec
Copy link
Contributor

tersec commented Oct 22, 2024

[NimScript] exec: nim c  --styleCheck:usages --styleCheck:error --verbosity:0 --hints:off --skipParentCfg --skipUserCfg --outdir:build --nimcache:build/nimcache -f --threads:on -d:chronicles_log_level=ERROR  -d:chronicles_log_level=TRACE -d:"chronicles_sinks=textlines[dynamic],json[dynamic]" --mm:refc tests/all
/home/runner/work/nim-json-rpc/nim-json-rpc/tests/testserverclient.nim(117, 1) template/generic instantiation of `suite` from here
/home/runner/work/nim-json-rpc/nim-json-rpc/tests/testserverclient.nim(118, 3) template/generic instantiation of `test` from here
/home/runner/.nimble/pkgs2/unittest2-0.2.3-6936b4e4676c9b37537d93f31cb8fc46f4ebaacb/unittest2.nim(1128, 24) template/generic instantiation of `failingOnExceptions` from here
/home/runner/.nimble/pkgs2/unittest2-0.2.3-6936b4e4676c9b37537d93f31cb8fc46f4ebaacb/unittest2.nim(1132, 26) template/generic instantiation of `failingOnExceptions` from here
/home/runner/work/nim-json-rpc/nim-json-rpc/tests/testserverclient.nim(130, 5) Warning: Raises Defect on future failure, fix your code and use asyncSpawn!; asyncCheck is deprecated [Deprecated]
/home/runner/work/nim-json-rpc/nim-json-rpc/build/nimcache/@m..@sjson_rpc@[email protected]: In function ‘serveHTTP__OOZjson95rpcZserversZhttpserver_u71’:
/home/runner/work/nim-json-rpc/nim-json-rpc/build/nimcache/@m..@sjson_rpc@[email protected]:1894:83: error: redeclaration of ‘sX60gensym105_’ with no linkage
 1894 |                                         tyObject_Result__wqhWq1B5QY9az9bOBYrpgR8Q sX60gensym105_;
      |                                                                                   ^~~~~~~~~~~~~~
/home/runner/work/nim-json-rpc/nim-json-rpc/build/nimcache/@m..@sjson_rpc@[email protected]:1892:83: note: previous declaration of ‘sX60gensym105_’ with type ‘tyObject_Result__wqhWq1B5QY9az9bOBYrpgR8Q’
 1892 |                                         tyObject_Result__wqhWq1B5QY9az9bOBYrpgR8Q sX60gensym105_;
      |                                                                                   ^~~~~~~~~~~~~~
Error: execution of an external compiler program 'gcc -c  -w -fmax-errors=3 -pthread -I/home/runner/.nimble/pkgs2/bearssl-0.2.5-550e6f9321b85de53bba9c0ffab9c95ffbe12ab3/bearssl/abi -I/home/runner/.nimble/pkgs2/bearssl-0.2.5-550e6f9321b85de53bba9c0ffab9c95ffbe12ab3/bearssl/abi/../csources/src/ -I/home/runner/.nimble/pkgs2/bearssl-0.2.5-550e6f9321b85de53bba9c0ffab9c95ffbe12ab3/bearssl/abi/../csources/inc/ -I/home/runner/.nimble/pkgs2/bearssl-0.2.5-550e6f9321b85de53bba9c0ffab9c95ffbe12ab3/bearssl/abi/../csources/tools/ -DBR_USE_UNIX_TIME=1 -DBR_USE_URANDOM=1 -DBR_LE_UNALIGNED=1 -DBR_64=1  -DBR_amd64=1 -DBR_INT128=1 -I/home/runner/.nimble/pkgs2/zlib-0.1.0-faccf2d54a8fe919aa2ac01b5af79a3b49b045fb/zlib/csources -DHAVE_UNISTD_H   -I/home/runner/work/nim-json-rpc/nim-json-rpc/nim/lib -I/home/runner/work/nim-json-rpc/nim-json-rpc/tests -o /home/runner/work/nim-json-rpc/nim-json-rpc/build/nimcache/@m..@sjson_rpc@[email protected] /home/runner/work/nim-json-rpc/nim-json-rpc/build/nimcache/@m..@sjson_rpc@[email protected]' failed with exit code: 1

That specific bug applies across versions, whereas this one only appears in devel, but the difference seems due to some details of how the async transformation works.

@metagn
Copy link
Contributor

metagn commented Oct 22, 2024

Thanks for minimizing (hopefully it's the same issue), more specifically it was triggered by nim-lang/Nim#24316: before it s was attached to the closure environment instead of being locally declared which worked around the redeclaration issue.

@arnetheduck arnetheduck merged commit 98a5efb into master Oct 22, 2024
13 of 18 checks passed
@arnetheduck arnetheduck deleted the fixes branch October 22, 2024 19:58
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.

3 participants