refactor: addition of waku_api/rest/builder.nim and reduce app.nim #2623
Conversation
|
You can find the image built from this PR at Built from 14f1673 |
|
You can find the image built from this PR at Built from 14f1673 |
| if app.restServer.isSome(): | ||
| await app.restServer.get().stop() | ||
| if app.conf.rest: | ||
| await app.restServer.stop() |
There was a problem hiding this comment.
Don't really see where app.restServer is assigned?
So when it stops it will result in segfault:
/home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(203) wakunode2
/home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncengine.nim(1210) runForever
/home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncengine.nim(1031) poll
/home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/ioselects/ioselectors_epoll.nim(643) selectInto2
../sysdeps/unix/sysv/linux/epoll_wait.c(30) epoll_wait
/home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(175) handleCtrlC
/home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(165) asyncStopper
/home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncfutures.nim(382) futureContinue
/home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(166) asyncStopper
/home/nzp/dev/status/nwaku-second/waku/factory/app.nim(320) stop
/home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncfutures.nim(382) futureContinue
/home/nzp/dev/status/nwaku-second/waku/factory/app.nim(321) stop
/home/nzp/dev/status/nwaku-second/waku/waku_api/rest/server.nim(191) stop
/home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncfutures.nim(382) futureContinue
/home/nzp/dev/status/nwaku-second/waku/waku_api/rest/server.nim(193) stop
/home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(194) handleSigsegv
Segmentation fault
There was a problem hiding this comment.
Don't really see where app.restServer is assigned? So when it stops it will result in segfault:
/home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(203) wakunode2 /home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncengine.nim(1210) runForever /home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncengine.nim(1031) poll /home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/ioselects/ioselectors_epoll.nim(643) selectInto2 ../sysdeps/unix/sysv/linux/epoll_wait.c(30) epoll_wait /home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(175) handleCtrlC /home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(165) asyncStopper /home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncfutures.nim(382) futureContinue /home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(166) asyncStopper /home/nzp/dev/status/nwaku-second/waku/factory/app.nim(320) stop /home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncfutures.nim(382) futureContinue /home/nzp/dev/status/nwaku-second/waku/factory/app.nim(321) stop /home/nzp/dev/status/nwaku-second/waku/waku_api/rest/server.nim(191) stop /home/nzp/dev/status/nwaku-second/vendor/nim-chronos/chronos/internal/asyncfutures.nim(382) futureContinue /home/nzp/dev/status/nwaku-second/waku/waku_api/rest/server.nim(193) stop /home/nzp/dev/status/nwaku-second/apps/wakunode2/wakunode2.nim(194) handleSigsegv Segmentation fault
Wow! Thanks for it!
I tried to tackle that in d448dd6
qq: how did you force that stop ? Very interesting validation :)
There was a problem hiding this comment.
It was easy, run with --rest=true and run node than press ctrl-c to finish.
|
|
||
| let address = conf.restAddress | ||
| let port = Port(conf.restPort + conf.portsShift) | ||
| let server = |
There was a problem hiding this comment.
I think in case conf.rest is false we shall not create the restServer.
There was a problem hiding this comment.
I think in case conf.rest is false we shall not create the restServer.
Good point and thanks for the comment! Nevertheless, I'm trying to respect the current implementation where we always create, at least, one restServer when calling startRestServerEsentials:
nwaku/apps/wakunode2/wakunode2.nim
Line 130 in 6d135b0
Line 333 in 6d135b0
There was a problem hiding this comment.
But why to run an http service and open port while it is explicitly told on cli not to do so?
I would not expose any port and consume resource in case it is not needed. WDYT?
| if app.restServer.isSome(): | ||
| await app.restServer.get().stop() | ||
| if app.conf.rest: | ||
| await app.restServer.stop() |
There was a problem hiding this comment.
It was easy, run with --rest=true and run node than press ctrl-c to finish.
|
|
||
| let address = conf.restAddress | ||
| let port = Port(conf.restPort + conf.portsShift) | ||
| let server = |
There was a problem hiding this comment.
But why to run an http service and open port while it is explicitly told on cli not to do so?
I would not expose any port and consume resource in case it is not needed. WDYT?
|
@NagyZoltanPeter - what do you mean in these questions?
|
@Ivansete-status We discussed in dm and you fixed. It was just about not to start restServer if configured --rest=false |
Description
Extract some REST logic from app.nim, aiming for simplify app.nim a little.
I will submit a couple of similar PRs simplifying app.nim.
note: this has the same goal as #2610 but I decided to submit a different one just to keep the original one as a temporary reference. I will drop the initial PR once this gets merged