Automatically create and wire form type services when possible

[rimas-kudelis]

The screenshot in the example here currently suggests that a form type service is being automatically created for every custom resource.

This is not true, but I think everyone would benefit if it actually were true. Sylius Standard automatically disables autowiring for all form types which extend AbstractResourceType. This means that we aren't getting any errors about Symfony's inability to autowire the constructor of AbstractResourceType, but whenever a form like that is actually used, the error still pops up, asking you to wire these two arguments manually.

It would be nice if the ResourceBundle's DI took care to wire these two arguments (class name and validation groups) for all forms which extend AbstractResourceType. This would allow us to avoid hardcoding the actual class name in the forms and would nicely complement the way other services (Controller, Factory, Manager, Repository) are created.

[ztellman]

Hi, this code is taken verbatim from clj-http, and every time an issue like this arises I call in @dakrone to make changes to his project which I can reflect here. If you'd like to submit PRs to both our projects, I'm sure that would be greatly appreciated by everyone.

[dakrone]

Thanks for the reproduction, I'll take a look at this.

[dakrone]

While aleph does use the same parse-transit method, it does diverge quite significantly with regard to the coerce-transit-body method. For example, aleph's:

https://github.com/ztellman/aleph/blob/3c425c848777d08f667729d570747e5ec1408393/src/aleph/http/client_middleware.clj#L867-L871

And clj-http's current version:

https://github.com/dakrone/clj-http/blob/90987a97bbd2413241da34aec9a698608106bd22/src/clj_http/client.clj#L359-L383

In particular, with https://github.com/dakrone/clj-http/commit/6574e0ac0143866c749ca31962e394a903b249ae I made some changes to the way that streams are coerced to handle empty streams a bit better. I can't say for certain whether it is the underlying cause, but since clj-http only ever passes a ByteArrayInputStream over an already-realized byte array, calling .available doesn't cause the problem in clj-http.

@ztellman not sure how you want to go about handling this, I think aleph has diverged a bit from clj-http so it's hard for me to say for certain the best way it should be fixed.

[ckarlsen84]

This doesn't happen when using clj-http.

I'm suspecting there is some weird async behaviour going on in aleph/netty, but unfortunately i'm not too familiar with the internals yet, so debugging is quite time consuming.

I've discovered another minimal case that fails every time

(require '[aleph.http :as http])
(require '[cognitect.transit :as transit])
(import '(java.io ByteArrayOutputStream))

(def server (http/start-server
              (fn [req]
                (let [out (ByteArrayOutputStream. 4096)
                      writer (transit/writer out :json)]
                  (transit/write writer {:foo :bar})
                  {:status 200
                   :headers {"content-type" "application/transit+json"
                             "transfer-encoding" "chunked"}
                   :body (.toString out)}))
              {:port 10000}))

(defn transit-test
  []
  (let [pool (http/connection-pool {})
        resp @(http/get "http://localhost:10000" {:as :transit+json
                                                  :pool pool})]
    (.shutdown pool)
    resp))

(transit-test)
=>
{:request-time 2,
 :aleph/keep-alive? true,
 :headers {"server" "Aleph/0.4.4",
           "content-type" "application/transit+json",
           "content-length" "22",
           "connection" "Keep-Alive",
           "transfer-encoding" "chunked",
           "date" "Wed, 14 Mar 2018 14:34:43 GMT"},
 :status 200,
 :connection-time 2,
 :body nil}

Potenial clue:

;; Server has received one single request ever
(.close server)

Mar 14, 2018 3:40:52 PM clojure.tools.logging$eval316$fn__319 invoke
SEVERE: error in invoke-callbacks
java.lang.RuntimeException: java.lang.IllegalStateException: already shutdown
	at io.aleph.dirigiste.Pool.addObject(Pool.java:277)
	at io.aleph.dirigiste.Pool.dispose(Pool.java:467)
	at aleph.flow$dispose.invokeStatic(flow.clj:101)
	at aleph.flow$dispose.invoke(flow.clj:98)
	at aleph.http$connection_pool$fn__6750$fn__6751.invoke(http.clj:153)
	at aleph.http.client$http_connection$fn__6679$fn__6692.invoke(client.clj:468)
	at manifold.utils$invoke_callbacks$fn__652.invoke(utils.clj:69)
	at manifold.utils$invoke_callbacks.invokeStatic(utils.clj:68)
	at manifold.utils$invoke_callbacks.invoke(utils.clj:65)
	at manifold.stream.default.Stream.markClosed(default.clj:41)
	at manifold.stream.default.Stream.close(default.clj:86)
	at aleph.http.client$client_handler$reify__6599.channelInactive(client.clj:165)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:245)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:231)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelInactive(AbstractChannelHandlerContext.java:224)
	at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelInactive(CombinedChannelDuplexHandler.java:420)
	at io.netty.handler.codec.ByteToMessageDecoder.channelInputClosed(ByteToMessageDecoder.java:377)
	at io.netty.handler.codec.ByteToMessageDecoder.channelInactive(ByteToMessageDecoder.java:342)
	at io.netty.handler.codec.http.HttpClientCodec$Decoder.channelInactive(HttpClientCodec.java:282)
	at io.netty.channel.CombinedChannelDuplexHandler.channelInactive(CombinedChannelDuplexHandler.java:223)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:245)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:231)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelInactive(AbstractChannelHandlerContext.java:224)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelInactive(DefaultChannelPipeline.java:1354)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:245)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:231)
	at io.netty.channel.DefaultChannelPipeline.fireChannelInactive(DefaultChannelPipeline.java:917)
	at io.netty.channel.AbstractChannel$AbstractUnsafe$8.run(AbstractChannel.java:822)
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:403)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:463)
	at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
	at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:138)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.IllegalStateException: already shutdown
	at io.aleph.dirigiste.Pool$Queue.put(Pool.java:100)
	at io.aleph.dirigiste.Pool$Queue.access$400(Pool.java:11)
	at io.aleph.dirigiste.Pool.addObject(Pool.java:273)
	... 33 more

nil

Another clue:

While messing around I added two println statements here that just prints out req and rsp and then it actually worked... I guess it added enough latency so the bytes got written to the input stream before reaching parse-transit...?

[ztellman]

@dakrone apologies for calling you in unnecessarily. I don't think Aleph has diverged so much as fallen behind your changes. I should do a pass over all the client middleware in the near future.

I'll look into this further.

[dakrone]

@dakrone apologies for calling you in unnecessarily.

No problem at all! Let me know if there's anything I can do to help

[kachayev]

@ztellman From what I can see the root of the problem is here: https://github.com/ztellman/byte-streams/blob/6d5dc09076b2e54086fa11e4ce5e096e9d6d900e/src/byte_streams/pushback_stream.clj#L101-L102: we create InputStream from SynchronizedPushbackByteStream with buffer-size set to 0. Meaning if (pos? (.available in)) was called before first put! is finished parse-transit just returns nil. And that's perfectly fine for from the pushback stream point of view, as available was never meant to be used to perform such checks:

Note that while some implementations of InputStream will return the total number of bytes in the stream, many will not. It is never correct to use the return value of this method to allocate a buffer intended to hold all data in this stream.

So, we can just remove .available check, the pushback stream short-circuits reads when underlying stream is closed.

[kachayev]

@ztellman I think it's safe now to close this issue 🤔

[ztellman]

@kachayev thanks for combing through all the old issues. If anything seems obviously complete, feel free to close it yourself.